[PATCH] ftp charset selection support
Hello, Currently there are a lot of ftp servers with different charsets and it is a problem to work with these hosts if local encoding differs, only for Russian there are over 10 different encoding and 3 of them are widely used. So I made a patch which allows to select remote server's encoding and recode URL into it. I've found that function for filename recoding already exists: filename_recode() in mp_msg.c, so I changed it in order to recode from arbitrary initial encoding to arbitrary final one. I extracted common part to filename_recode_common() and added iconv error handling routines (otherwise iconv silently fails if e.g. incorrect encoding is specified.). iconv_t handlers are separate for each "branch" of convertios, because both strcmp() and iconv_open()/iconv_close() are expensive operations to be performed each time. This is what the first patch supposed to do. The second patch is just cosmetics required after the first patch. The third patch is ftp URL recoding support itself utilizing the first patch. And the fourth patch is man page update describes new -ftp-charset option. Currently user must specify encoding on his/her own, but in several cases it is possible to autodetect encoding: 1) Negotiation with server using LANG feature (rfc2640) if server supports it. 2) Alternatively try to server's guess encoding by descendingly parsing directories in URL (starting from /) in order to identify encoding using ENCA. This will be implemented later, I hope.
On Sat, Apr 12, 2008 at 05:45:59AM +0400, Andrew Savchenko wrote:
I've found that function for filename recoding already exists: filename_recode() in mp_msg.c, so I changed it in order to recode from arbitrary initial encoding to arbitrary final one.
IMO, since filename_recode is not thread safe, it should only be used for _printing_ stuff, never for actually important code, and preferably even that only from as few places as possible.
Hi, On Saturday 12 April 2008 12:42, Reimar Döffinger wrote:
On Sat, Apr 12, 2008 at 05:45:59AM +0400, Andrew Savchenko wrote:
I've found that function for filename recoding already exists: filename_recode() in mp_msg.c, so I changed it in order to recode from arbitrary initial encoding to arbitrary final one.
IMO, since filename_recode is not thread safe, it should only be used for _printing_ stuff, never for actually important code, and preferably even that only from as few places as possible.
Yes, original filename_recode() isn't thread safe, but I moved static iconv_t handle out of the filename_recode_common() and, when used for ftp purposes, I save buffer with recoded filename to an external buffer. Moreover in my next patch I will move static buffer for recoded_filename out of filename_recode_common() to filename_recode(). Thus filename_recode_common() will be thread safe completely.
On Sat, 12 Apr 2008 05:45:59 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hello,
Currently there are a lot of ftp servers with different charsets and it is a problem to work with these hosts if local encoding differs, only for Russian there are over 10 different encoding and 3 of them are widely used. So I made a patch which allows to select remote server's encoding and recode URL into it.
Just out of interest, how is this problem handled in HTML? Is it needed to write the document in the encoding used by the FTP server one link to? Generally I'm OK with the idea of this patch, but we might want to also take a look at the URL/filename encoding problem in general. The problem have 2 side, source encoding and destination encoding. As source we currently have: * command line: the terminal encoding will do * slave command: terminal encoding or fs encoding? * playlist: some format/transport protocol might specify it, fallback on local fs encoding? On the destination side only the stream itself can tell what is needed. Using a common format, like UTF-8, would be tempting; but AFAIK back and forth conversion is not reliable with all encoding. The best would then probably be to simply pass the encoding along with the URL/filename when opening the stream. Suggestions welcome :)
I've found that function for filename recoding already exists: filename_recode() in mp_msg.c, so I changed it in order to recode from arbitrary initial encoding to arbitrary final one. I extracted common part to filename_recode_common() and added iconv error handling routines (otherwise iconv silently fails if e.g. incorrect encoding is specified.). iconv_t handlers are separate for each "branch" of convertios, because both strcmp() and iconv_open()/iconv_close() are expensive operations to be performed each time. This is what the first patch supposed to do.
+#ifdef USE_ICONV +#include <iconv.h> +#endif /* USE_ICONV */ + +/* maximum message length of mp_msg */ +#define MSGSIZE_MAX 3072 + +const char* filename_recode_common(iconv_t inv_msgiconv, + const char* filename,const char *tocode, const char *fromcode); +
This is going to break if iconv is unavailable. The function name could also be better. This function is not specific to filenames and _common say very little. mp_recode_to()? I would suggest making this function static and adding a wrapper that hide iconv. That would allow mp_msg to keep using a static iconv context and make things simpler for other uses.
-/* maximum message length of mp_msg */ -#define MSGSIZE_MAX 3072 -
Why do you move this define out of mp_msg.c?
The second patch is just cosmetics required after the first patch.
Fine.
The third patch is ftp URL recoding support itself utilizing the first patch.
+/* defined in stream_ftp.c */ +#ifdef USE_ICONV +extern char *ftp_charset; +#endif /* USE_ICONV */
No need to put extern declarations under #ifdef.
+#ifdef USE_ICONV +#include <iconv.h> +#include <string.h>
The need for iconv should be abstracted away.
+#ifdef USE_ICONV
Get rid of the #ifdef.
+ //recode filename if needed + if (ftp_charset) { + mp_msg(MSGT_OPEN, MSGL_V, "[ftp] recoding filename %s from %s to %s\n", + p->filename, get_term_charset(), ftp_charset); + + iconv_t iconv_filename = (iconv_t)(-1); + /* There is no guarantee that filename_recode_common wouldn't + * be called during playback. Thus buffer must be copied to + * p->filename with appropriate memory reallocation. */ + char *buf = filename_recode_common(iconv_filename, + p->filename, ftp_charset, get_term_charset());
Don't use declarations in middle of code blocks.
+ size_t len = strlen(buf) + 1; + p->filename = realloc(p->filename, len); + memcpy(p->filename, buf, len); + } +#endif /* USE_ICONV */
This is leaking memory.
And the fourth patch is man page update describes new -ftp-charset option.
Currently user must specify encoding on his/her own, but in several cases it is possible to autodetect encoding: 1) Negotiation with server using LANG feature (rfc2640) if server supports it. 2) Alternatively try to server's guess encoding by descendingly parsing directories in URL (starting from /) in order to identify encoding using ENCA.
This will be implemented later, I hope.
Patch welcome :) Albeu
Hi, On Saturday 12 April 2008 14:30, Alban Bedel wrote:
Andrew Savchenko <Bircoph@list.ru> wrote:
Hello,
Currently there are a lot of ftp servers with different charsets and it is a problem to work with these hosts if local encoding differs, only for Russian there are over 10 different encoding and 3 of them are widely used. So I made a patch which allows to select remote server's encoding and recode URL into it.
Just out of interest, how is this problem handled in HTML? Is it needed to write the document in the encoding used by the FTP server one link to?
Yes, it is required to be written in ftp server's encoding. I have an ffsearch server on my host and encountered this problem long time ago... Originally ftp supported only ASCII charset (rfc959) and there was no problems, but afterwards file names in national encodings began to occur, and all this mess begins. Well, UTF-8 will be a fix after all, but this wouldn't be soon...
Generally I'm OK with the idea of this patch, but we might want to also take a look at the URL/filename encoding problem in general. The problem have 2 side, source encoding and destination encoding. As source we currently have:
* command line: the terminal encoding will do * slave command: terminal encoding or fs encoding?
IMHO if mplayer reads slave commands from stdin, it should use terminal encoding. If it reads them from file it should use FS encoding. Maybe it will be better to introduce some input_charset internal variable to make input-encoding-dependent functions to be independent of type of input: where we use -slave from file or normal stdin, or from command line. Perhaps it is worthy to make this variable configurable by the user.
* playlist: some format/transport protocol might specify it, fallback on local fs encoding?
Perhaps in case of transport protocol it will be reasonable to use server's encoding as fallback, and for local playlist use local encoding. Also, ENCA seems to be a good choice as fallback for this and previous issue. Currently it is used in subtitle routines and saves me a lot of time. Only Russian subtitles I own are present in 5 different encodings and it would be a pain to deal with them if not ENCA. Of course this approach in not ideal and may fail, but this is better than nothing and seems to work perfectly in the most of cases.
On the destination side only the stream itself can tell what is needed. Using a common format, like UTF-8, would be tempting;
Are you talking about network streams or about an internal streams inside mplayer? Most likely that network streams are the subject. I dunno exactly, but I'm not sure that all type of streams support encoding information and all network servers set it properly.
but AFAIK back and forth conversion is not reliable with all encoding.
At least it seems to be reliable for cyrillic and CJK charsets, I can't tell about the rest.
The best would then probably be to simply pass the encoding along with the URL/filename when opening the stream. Suggestions welcome :)
And why not introduce new command line option? Inventing new URL schemes seems not good from my standpoint, but this may be implemented optionally. And do not forget about ENCA. We can use the same approach as in subcp: enca[:fallback language[:fallbach charset]].
+#ifdef USE_ICONV +#include <iconv.h> +#endif /* USE_ICONV */ + +/* maximum message length of mp_msg */ +#define MSGSIZE_MAX 3072 + +const char* filename_recode_common(iconv_t inv_msgiconv, + const char* filename,const char *tocode, const char *fromcode); +
This is going to break if iconv is unavailable.
Will fix it.
The function name could also be better. This function is not specific to filenames and _common say very little. mp_recode_to()?
Yes, it is a good choice. And what is more this function becomes a general i/o routine rather than mp_msg specific stuff. I think it is better to move this function somewhere else, but where exactly? I'm in doubt in finding appropriate file to place this function.
I would suggest making this function static and adding a wrapper that hide iconv. That would allow mp_msg to keep using a static iconv context and make things simpler for other uses.
Isn't filename_recode() is an appropriate wrapper to mp_recode_to() (using new proposed function name)? It handles iconv context for mp_recode_to() currently. I have no objections against making these functions static, so it will be done. Also I want to extract storage buffer for recoded_name from mp_recode_to() in order to make this function thread safe.
-/* maximum message length of mp_msg */ -#define MSGSIZE_MAX 3072 -
Why do you move this define out of mp_msg.c?
It is a remnant from earlier code changes. Will be removed.
+/* defined in stream_ftp.c */ +#ifdef USE_ICONV +extern char *ftp_charset; +#endif /* USE_ICONV */
No need to put extern declarations under #ifdef.
Ok, but I'm surprised by this language feature ;-).
+#ifdef USE_ICONV +#include <iconv.h> +#include <string.h>
The need for iconv should be abstracted away.
+#ifdef USE_ICONV
Get rid of the #ifdef.
ok
+ //recode filename if needed + if (ftp_charset) { + mp_msg(MSGT_OPEN, MSGL_V, "[ftp] recoding filename %s from %s to %s\n", + p->filename, get_term_charset(), ftp_charset); + + iconv_t iconv_filename = (iconv_t)(-1); + /* There is no guarantee that filename_recode_common wouldn't + * be called during playback. Thus buffer must be copied to + * p->filename with appropriate memory reallocation. */ + char *buf = filename_recode_common(iconv_filename, + p->filename, ftp_charset, get_term_charset());
Don't use declarations in middle of code blocks.
Will be enough to place declaration just after "{" clause after "if" statement?
+ size_t len = strlen(buf) + 1; + p->filename = realloc(p->filename, len); + memcpy(p->filename, buf, len); + } +#endif /* USE_ICONV */
This is leaking memory.
Why?! This is realloc(), not malloc(). It will reuse (or free() and malloc()) memory of p->filename. At the end, close_f() will free this memory inside m_struct_free() call in the same way as it will be freed in the case of original p->filename pointer. From the point of m_option_free() it should not be any difference between handling the old and the new pointer.
Currently user must specify encoding on his/her own, but in several cases it is possible to autodetect encoding: 1) Negotiation with server using LANG feature (rfc2640) if server supports it. 2) Alternatively try to server's guess encoding by descendingly parsing directories in URL (starting from /) in order to identify encoding using ENCA.
This will be implemented later, I hope.
Patch welcome :)
I'll work on patches later. Today I feel myself too tired... It would be great to hear from you some words about (possible) memory leak and mp_recode_to() issues before the new patch will be made. Well, I have look over some RFC's concerning ftp, and some real servers... I have no idea what ftp server supports LANG extension. Even my favourite vsftpd doesn't do this. Anyway I can use reference code from lftp. Also I have found another interesting feature which will be usefull: ff server supports FEAT and OPTS UTF8 ON, the last command can be used to established utf8 filename transmission for sure; vsftpd do this by default. With best regards, Andrew
On Sat, 12 Apr 2008 22:29:14 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hi,
On Saturday 12 April 2008 14:30, Alban Bedel wrote:
Andrew Savchenko <Bircoph@list.ru> wrote: Generally I'm OK with the idea of this patch, but we might want to also take a look at the URL/filename encoding problem in general. The problem have 2 side, source encoding and destination encoding. As source we currently have:
* command line: the terminal encoding will do * slave command: terminal encoding or fs encoding?
IMHO if mplayer reads slave commands from stdin, it should use terminal encoding. If it reads them from file it should use FS encoding. Maybe it will be better to introduce some input_charset internal variable to make input-encoding-dependent functions to be independent of type of input: where we use -slave from file or normal stdin, or from command line. Perhaps it is worthy to make this variable configurable by the user.
Yeah, the encoding used for slave commands should be configurable. The problem is the default, I dunno if it would be too wise to have different default depending on the fd type. It might lead to quiet some confusions.
* playlist: some format/transport protocol might specify it, fallback on local fs encoding?
Perhaps in case of transport protocol it will be reasonable to use server's encoding as fallback, and for local playlist use local encoding.
That what I was thinking, although again we probably want to have the "local encoding" configurable.
Also, ENCA seems to be a good choice as fallback for this and previous issue. Currently it is used in subtitle routines and saves me a lot of time. Only Russian subtitles I own are present in 5 different encodings and it would be a pain to deal with them if not ENCA. Of course this approach in not ideal and may fail, but this is better than nothing and seems to work perfectly in the most of cases.
The problem is that most path are relatively short, it make guessing a lot harder.
On the destination side only the stream itself can tell what is needed. Using a common format, like UTF-8, would be tempting;
Are you talking about network streams or about an internal streams inside mplayer? Most likely that network streams are the subject. I dunno exactly, but I'm not sure that all type of streams support encoding information and all network servers set it properly.
I'm talking about the MPlayer stream types. For ex. the local fs stream need a path in the local encoding, the ftp stream need to use the remote server encoding, some protocol might mandate UTF-8, etc
but AFAIK back and forth conversion is not reliable with all encoding.
At least it seems to be reliable for cyrillic and CJK charsets, I can't tell about the rest.
AFAIK CJK are the problem, bcs with the Han unification different chars in the legacy encodings give the same unicode char. Obviously you can't then convert back from unicode to legacy without some loss.
The best would then probably be to simply pass the encoding along with the URL/filename when opening the stream. Suggestions welcome :)
And why not introduce new command line option? Inventing new URL schemes seems not good from my standpoint, but this may be implemented optionally. And do not forget about ENCA. We can use the same approach as in subcp: enca[:fallback language[:fallbach charset]].
That's what I meant. To be more precise I thought about adding an extra parameter to open_stream() to indicate the filename encoding.
+ //recode filename if needed + if (ftp_charset) { + mp_msg(MSGT_OPEN, MSGL_V, "[ftp] recoding filename %s from %s to %s\n", + p->filename, get_term_charset(), ftp_charset); + + iconv_t iconv_filename = (iconv_t)(-1); + /* There is no guarantee that filename_recode_common wouldn't + * be called during playback. Thus buffer must be copied to + * p->filename with appropriate memory reallocation. */ + char *buf = filename_recode_common(iconv_filename, + p->filename, ftp_charset, get_term_charset());
Don't use declarations in middle of code blocks.
Will be enough to place declaration just after "{" clause after "if" statement?
Sure.
+ size_t len = strlen(buf) + 1; + p->filename = realloc(p->filename, len); + memcpy(p->filename, buf, len); + } +#endif /* USE_ICONV */
This is leaking memory.
Why?! This is realloc(), not malloc(). It will reuse (or free() and malloc()) memory of p->filename. At the end, close_f() will free this memory inside m_struct_free() call in the same way as it will be freed in the case of original p->filename pointer. From the point of m_option_free() it should not be any difference between handling the old and the new pointer.
The iconv context is not freed AFAICT. Albeu
Hi, On Sunday 13 April 2008 17:12, Alban Bedel wrote: [...]
Yeah, the encoding used for slave commands should be configurable. The problem is the default, I dunno if it would be too wise to have different default depending on the fd type. It might lead to quiet some confusions.
I dunno either. Having them both may be confusing, having just single one may be inconvenient.
* playlist: some format/transport protocol might specify it, fallback on local fs encoding?
Perhaps in case of transport protocol it will be reasonable to use server's encoding as fallback, and for local playlist use local encoding.
That what I was thinking, although again we probably want to have the "local encoding" configurable.
Ok, I think this would be easy to implement.
Also, ENCA seems to be a good choice as fallback for this and previous issue. Currently it is used in subtitle routines and saves me a lot of time. Only Russian subtitles I own are present in 5 different encodings and it would be a pain to deal with them if not ENCA. Of course this approach in not ideal and may fail, but this is better than nothing and seems to work perfectly in the most of cases.
The problem is that most path are relatively short, it make guessing a lot harder.
Well, I made small test: created some files with single word, converted them to different encodings and made ENCA to guess encoding for that files. 2/3 of tests were passed successfull, this is still better than nothing. Moreover, another trick can be made: root directory listing may be obtained for the root directory (if server allows to list dirs of course) and ENCA may be used against this listing. However it is possible that root directory listing doesn't contain enough symbols or contain English characters only, so this procedure should be done recursively; as a drawback this may lead into perceptible lag. Futher more such kind of autodetection may lead to playback of another file. In some encodings a lot of symbols are interchangeable, but with different codes. For example it is possible to create filename in cp1251 charset, convert it into koi8r, but write it again into cp1251 encoding -- hence we will have two file both of them may be recoded in the single filename. However, probability of this case is very low.
but AFAIK back and forth conversion is not reliable with all encoding.
At least it seems to be reliable for cyrillic and CJK charsets, I can't tell about the rest.
AFAIK CJK are the problem, bcs with the Han unification different chars in the legacy encodings give the same unicode char.
Hmm, I'm surprised with this. I have work only with Japan charsets so haven't encountered this problem. It is more interesting why unicode doesn't handle this chars as separate characters. Afaik UTF-8 is supposed to be able to contain any written character on the Earth by design.
And why not introduce new command line option? Inventing new URL schemes seems not good from my standpoint, but this may be implemented optionally. And do not forget about ENCA. We can use the same approach as in subcp: enca[:fallback language[:fallbach charset]].
That's what I meant. To be more precise I thought about adding an extra parameter to open_stream() to indicate the filename encoding.
Seconded.
Don't use declarations in middle of code blocks.
Will be enough to place declaration just after "{" clause after "if" statement?
Sure.
Fixed.
This is leaking memory.
Why?! This is realloc(), not malloc(). It will reuse (or free() and malloc()) memory of p->filename. At the end, close_f() will free this memory inside m_struct_free() call in the same way as it will be freed in the case of original p->filename pointer. From the point of m_option_free() it should not be any difference between handling the old and the new pointer.
The iconv context is not freed AFAICT.
Fixed. The first patch contains mp_recode_to() implementation. Now it uses no static buffer and can grow buffer as needed. Static buffer was acceptable for text messages, but playback failure due to insufficient buffer size is not acceptable. Now each caller function must handle its own buffer and iconv_t handle on its own. The second patch is -ftp-charset implementation itself with an appropriate documentation. With best regards, Andrew
On Sat, Apr 19, 2008 at 11:18:42PM +0400, Andrew Savchenko wrote:
--- mplayer_base/mp_msg.h 2008-03-08 02:46:16.000000000 +0300 +++ mplayer/mp_msg.h 2008-04-19 21:47:21.000000000 +0400 @@ -127,6 +127,21 @@ # endif #endif /* __GNUC__ */
-const char* filename_recode(const char* filename); +#ifdef USE_ICONV +#include <iconv.h> + +/* Recode filename from <tocode> to <fromcode> charset. + * + * *icnv -- iconv handler, unique for caller; + * *buf -- buffer for recoded result, must be allocated outside, + * but may be grown inside the function; + * *buf_size -- size of buffer to use. */ +const char* mp_recode_to(iconv_t* const icnv, + const char* const filename, + const char* const tocode, const char* const fromcode, + char** const buf, size_t* const buf_size); +#endif /* USE_ICONV */ + +const char* filename_recode(const char* const filename);
I see now point in about half of those consts, they IMO just obfuscate the code, and esp. the declaration of filename_recode should not change.
--- mplayer_base/mp_msg.c 2008-04-12 20:24:22.000000000 +0400 +++ mplayer/mp_msg.c 2008-04-19 21:46:20.000000000 +0400 @@ -31,38 +31,74 @@ int mp_msg_module = 0; #ifdef USE_ICONV char *mp_msg_charset = NULL; -static char *old_charset = NULL; -static iconv_t msgiconv; -#endif - -const char* filename_recode(const char* filename) +char *old_charset = NULL; +iconv_t msgiconv; +iconv_t inv_msgiconv = (iconv_t)(-1);
Why removing the static? Why moving those variables out instead of leaving it in the filename_recode function?
+char *recoded_filename; +size_t recoded_filename_size = MSGSIZE_MAX; + +// recode filename from <tocode> to <fromcode> charset +const char* mp_recode_to(iconv_t* const icnv, + const char* const filename, + const char* const tocode, const char* const fromcode, + char** const buf, size_t* const buf_size) { -#if !defined(USE_ICONV) || !defined(MSG_CHARSET) - return filename; -#else
And e.g. this (and a bit of other code) still exists after the patch, by simply moving the new filename_recode function before the mp_recode_to quite a bit of diff should disappear and make this much, much easier to understand.
Hi, On Sunday 20 April 2008 00:31, Reimar Döffinger wrote:
On Sat, Apr 19, 2008 at 11:18:42PM +0400, Andrew Savchenko wrote:
--- mplayer_base/mp_msg.h 2008-03-08 02:46:16.000000000 +0300 +++ mplayer/mp_msg.h 2008-04-19 21:47:21.000000000 +0400 @@ -127,6 +127,21 @@ # endif #endif /* __GNUC__ */
-const char* filename_recode(const char* filename); +#ifdef USE_ICONV +#include <iconv.h> + +/* Recode filename from <tocode> to <fromcode> charset. + * + * *icnv -- iconv handler, unique for caller; + * *buf -- buffer for recoded result, must be allocated outside, + * but may be grown inside the function; + * *buf_size -- size of buffer to use. */ +const char* mp_recode_to(iconv_t* const icnv, + const char* const filename, + const char* const tocode, const char* const fromcode, + char** const buf, size_t* const buf_size); +#endif /* USE_ICONV */ + +const char* filename_recode(const char* const filename);
I see now point in about half of those consts, they IMO just obfuscate the code,
They are harmless, besides const qualifier protects from some development errors and may help compiler to optimize code or may not, but nevertheless they change nothing in the worst case. The only drawback is a little code readability decreasing, so perhaps they are worth to be used.
and esp. the declaration of filename_recode should not change.
I agree with you here, if stable function interface may be untouched, it should be untouched.
--- mplayer_base/mp_msg.c 2008-04-12 20:24:22.000000000 +0400 +++ mplayer/mp_msg.c 2008-04-19 21:46:20.000000000 +0400 @@ -31,38 +31,74 @@ int mp_msg_module = 0; #ifdef USE_ICONV char *mp_msg_charset = NULL; -static char *old_charset = NULL; -static iconv_t msgiconv; -#endif - -const char* filename_recode(const char* filename) +char *old_charset = NULL; +iconv_t msgiconv; +iconv_t inv_msgiconv = (iconv_t)(-1);
Why removing the static?
They are in global scope, so static by default. Static declarations are absolutely useless here, so they was removed. Well, if you insist I'll keep them, anyway they change nothing.
Why moving those variables out instead of leaving it in the filename_recode function?
No reason ;-/, moved it back.
+char *recoded_filename; +size_t recoded_filename_size = MSGSIZE_MAX; + +// recode filename from <tocode> to <fromcode> charset +const char* mp_recode_to(iconv_t* const icnv, + const char* const filename, + const char* const tocode, const char* const fromcode, + char** const buf, size_t* const buf_size) { -#if !defined(USE_ICONV) || !defined(MSG_CHARSET) - return filename; -#else
And e.g. this (and a bit of other code) still exists after the patch, by simply moving the new filename_recode function before the mp_recode_to quite a bit of diff should disappear and make this much, much easier to understand.
Done, this patch was really hard to read. Here is the updated patch for mp_msg.[ch]. The second patch is unchanged. With best regards, Andrew
Hi, On Sunday 20 April 2008 06:11, Andrew Savchenko wrote: [...]
Here is the updated patch for mp_msg.[ch]. The second patch is unchanged.
Err, can someone be so kind as to review this patch?
On Sat, 26 Apr 2008 17:29:01 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hi,
On Sunday 20 April 2008 06:11, Andrew Savchenko wrote: [...]
Here is the updated patch for mp_msg.[ch]. The second patch is unchanged.
Err, can someone be so kind as to review this patch?
i notice you have sent a bunch of patches that were committed. would you like to have mplayer svn commit access ? if so, please send a mail to root@mphq with your desired commit nickname. -compn
Hi, On Tuesday 29 April 2008 20:55, compn wrote: [...]
i notice you have sent a bunch of patches that were committed. would you like to have mplayer svn commit access ?
Yes, I consider it in an honour.
if so, please send a mail to root@mphq with your desired commit nickname.
Done.
On Sun, Apr 20, 2008 at 06:11:43AM +0400, Andrew Savchenko wrote:
On Sunday 20 April 2008 00:31, Reimar Döffinger wrote:
+const char* mp_recode_to(iconv_t* const icnv, + const char* const filename, + const char* const tocode, const char* const fromcode, + char** const buf, size_t* const buf_size); +#endif /* USE_ICONV */ + +const char* filename_recode(const char* const filename);
I see now point in about half of those consts, they IMO just obfuscate the code,
They are harmless, besides const qualifier protects from some development errors and may help compiler to optimize code or may not, but nevertheless they change nothing in the worst case. The only drawback is a little code readability decreasing, so perhaps they are worth to be used.
At least the second const for filename I can not really see much of a point in.
--- mplayer_base/mp_msg.c 2008-04-12 20:24:22.000000000 +0400 +++ mplayer/mp_msg.c 2008-04-19 21:46:20.000000000 +0400 @@ -31,38 +31,74 @@ int mp_msg_module = 0; #ifdef USE_ICONV char *mp_msg_charset = NULL; -static char *old_charset = NULL; -static iconv_t msgiconv; -#endif - -const char* filename_recode(const char* filename) +char *old_charset = NULL; +iconv_t msgiconv; +iconv_t inv_msgiconv = (iconv_t)(-1);
Why removing the static?
They are in global scope, so static by default. Static declarations are absolutely useless here, so they was removed. Well, if you insist I'll keep them, anyway they change nothing.
Read up on C. They are file-local scope with static and project-global without, which is a big difference.
--- mplayer_base/mp_msg.c 2008-04-12 20:24:22.000000000 +0400 +++ mplayer/mp_msg.c 2008-04-20 05:53:40.000000000 +0400 @@ -33,7 +33,9 @@ char *mp_msg_charset = NULL; static char *old_charset = NULL; static iconv_t msgiconv; -#endif +char *recoded_filename; +size_t recoded_filename_size = MSGSIZE_MAX; +#endif /* USE_ICONV */
And thus these should be static as well.
+#ifdef USE_ICONV +// recode filename from <tocode> to <fromcode> charset +const char* mp_recode_to(iconv_t* const icnv,
Function comments should be doxygen-compatible, though if the one in .h already is just remove this one IMO.
+ max_path = *buf_size - 1; + precoded = *buf; + while(1) { + if (iconv(*icnv, &filename, &filename_len, + &precoded, &max_path) == (size_t)(-1)) {
This fails in all kinds of ways for *buf_size == 0
+ // if buffer size is not enough, sufficient memory + // must be allocated; + if (errno == E2BIG) { + unsigned int diff = precoded - *buf; + // double buffer + max_path += *buf_size; + *buf_size <<= 1; + *buf = realloc(*buf, *buf_size); + precoded = (unsigned int)(*buf) + diff;
This as well is very wrong. It is just wrong on 64 bit systems, it is possibly exploitable (since it does not check for *buf == NULL) and it seems overcomplicated to me anyway, IMO just restart from scratch by calling iconv(*icnv, NULL, NULL, NULL, NULL); It's probably a good idea to do this for all errors.
@@ -77,6 +115,9 @@ mp_msg_charset = getenv("MPLAYER_CHARSET"); if (!mp_msg_charset) mp_msg_charset = get_term_charset(); + // allocate initial buffer for filename conversion + // via filename_recode() + recoded_filename = malloc(recoded_filename_size);
IMO fix the code to work with buf = NULL and buf_size = 0 and get rid of this. I have not reviewed the other patch. Greetings, Reimar Döffinger
Hi, On Tuesday 29 April 2008 21:49, Reimar Döffinger wrote: [...]
I see now point in about half of those consts, they IMO just obfuscate the code,
They are harmless, besides const qualifier protects from some development errors and may help compiler to optimize code or may not, but nevertheless they change nothing in the worst case. The only drawback is a little code readability decreasing, so perhaps they are worth to be used.
At least the second const for filename I can not really see much of a point in.
Ok, I'll remove it.
Why removing the static?
They are in global scope, so static by default. Static declarations are absolutely useless here, so they was removed. Well, if you insist I'll keep them, anyway they change nothing.
Read up on C. They are file-local scope with static and project-global without, which is a big difference.
Yes, I missed that; static qualifiers will be kept.
--- mplayer_base/mp_msg.c 2008-04-12 20:24:22.000000000 +0400 +++ mplayer/mp_msg.c 2008-04-20 05:53:40.000000000 +0400 @@ -33,7 +33,9 @@ char *mp_msg_charset = NULL; static char *old_charset = NULL; static iconv_t msgiconv; -#endif +char *recoded_filename; +size_t recoded_filename_size = MSGSIZE_MAX; +#endif /* USE_ICONV */
And thus these should be static as well.
Done.
+#ifdef USE_ICONV +// recode filename from <tocode> to <fromcode> charset +const char* mp_recode_to(iconv_t* const icnv,
Function comments should be doxygen-compatible, though if the one in .h already is just remove this one IMO.
I'll make comment in mp_msg.h doxy-compatible.
+ max_path = *buf_size - 1; + precoded = *buf; + while(1) { + if (iconv(*icnv, &filename, &filename_len, + &precoded, &max_path) == (size_t)(-1)) {
This fails in all kinds of ways for *buf_size == 0
Surely. This code was designed with the expectation of mp_recode_to() will never be called with NULL, or improperly allocated buffer, or zero buf_size. This was done in order to avoid extra checks, and in current code, where buffer is always allocated prior mp_recode_to() call, this should work fine. However, if mp_recode_to() fault-tolerance is required, I'll do this. Performance shouldn't be an issue for this function.
+ // if buffer size is not enough, sufficient memory + // must be allocated; + if (errno == E2BIG) { + unsigned int diff = precoded - *buf; + // double buffer + max_path += *buf_size; + *buf_size <<= 1; + *buf = realloc(*buf, *buf_size); + precoded = (unsigned int)(*buf) + diff;
This as well is very wrong. It is just wrong on 64 bit systems, it is possibly exploitable (since it does not check for *buf == NULL) and it seems overcomplicated to me anyway, IMO just restart from scratch by calling iconv(*icnv, NULL, NULL, NULL, NULL);
Well, I'm thinking about offset calculation in another way... I can use memcpy of course, but this seems ugly for me. I don't want to recall iconv() from scratch due to performance issues: this function may be very slow.
It's probably a good idea to do this for all errors.
At least for EILSEQ error code this will be a problem. EILSEQ occurs when invalid multibyte sequence was encountered, so buffer will be grown until out of memory. Perhaps we should just copy the problem character as is and issue a warning? This is still better than nothing.
@@ -77,6 +115,9 @@ mp_msg_charset = getenv("MPLAYER_CHARSET"); if (!mp_msg_charset) mp_msg_charset = get_term_charset(); + // allocate initial buffer for filename conversion + // via filename_recode() + recoded_filename = malloc(recoded_filename_size);
IMO fix the code to work with buf = NULL and buf_size = 0 and get rid of this. I have not reviewed the other patch.
Ok. Due to technical reasons I may be off-line during next two or three weeks, so pardon for this delay. With best regards, Andrew
On Fri, May 02, 2008 at 04:29:07PM +0400, Andrew Savchenko wrote:
This as well is very wrong. It is just wrong on 64 bit systems, it is possibly exploitable (since it does not check for *buf == NULL) and it seems overcomplicated to me anyway, IMO just restart from scratch by calling iconv(*icnv, NULL, NULL, NULL, NULL);
Well, I'm thinking about offset calculation in another way... I can use memcpy of course, but this seems ugly for me. I don't want to recall iconv() from scratch due to performance issues: this function may be very slow.
This function (at least so far) is basically never called during playback so unless you expect iconv to talk almost 0.5 s there is not really an issue. Also it only happens when the buffer is too small which should be quite rare as well.
It's probably a good idea to do this for all errors.
At least for EILSEQ error code this will be a problem. EILSEQ occurs when invalid multibyte sequence was encountered, so buffer will be grown until out of memory. Perhaps we should just copy the problem character as is and issue a warning? This is still better than nothing.
You misunderstood me: You should not increase buffer size on other errors, but I think it would be a good idea to reset iconv state on all errors. Greetings, Reimar Döffinger
participants (4)
-
Alban Bedel -
Andrew Savchenko -
compn -
Reimar Döffinger