[MPlayer-dev-eng] [PATCH] ftp charset selection support
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Tue Apr 29 19:49:21 CEST 2008
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
More information about the MPlayer-dev-eng
mailing list