[MPlayer-dev-eng] [PATCH] Add support for directory-wide configuration overrides V4.1

[Reimar Döffinger]

On Wed, Sep 02, 2009 at 09:09:03PM +0300, Christian P. Schmidt wrote:
> Reimar Döffinger wrote:
> > On Mon, Aug 31, 2009 at 06:02:05PM +0300, Christian P. Schmidt wrote:
> >> @@ -931,13 +932,11 @@
> >>  {
> >>      char *confpath;
> >>      char cfg[strlen(file)+10];
> >> +    char dircfg[strlen(file)+14];
> > 
> > Can be moved inside the use_filedir_conf block.
> 
> Done, however I personally dislike this way of coding.

In this case it matters little or simply does not apply, but putting
variables at the right level greatly simplifies things like checking
if it was correctly freed, finding out where a free can be placed safely
and if and how it can be safely modified in a way that destroys the
original value (e.g. a while loop that counts the original variable
down). The further up you place the declarations the more code you'd
have to review to do these kind of changes, thus increasing the
maintenance burden.
Of course as with most programming styles this is always something
disputable.

> >> @@ -952,6 +951,15 @@
> >>      else
> >>  	name++;
> >>  
> >> +    if (use_filedir_conf) {
> >> +        av_strlcpy(dircfg, cfg, name-cfg+1);
> >> +        strcat(dircfg, "mplayer.conf");
> > 
> > Even if you don't like my approach with a backup variable, I'd consider
> > strcpy(dircfg, cfg);
> > strcpy(dircfg + (name - cfg), "mplayer.conf");
> > clearer.
> 
> Done, however I think that the old code is more verbose, as it literally
> says "copy cfg up to the (file-)name into dircfg, than append mplayer.conf".

Well, "copy cfg into dircfg and overwrite the file name" is not more
complex either and almost an equally "literal" translation of the code.
What I think made your version non-obvious was the strlcpy and the thus
needed +1, strncpy and would have been clearer but would have needed an
extra 0-termination.

> I didn't touch the string size related things because I'm not sure where
> the filename actually can come from - if it's from command line it's
> unlikely to be that large, from playlists however it might be possible
> to put larger filelengths in to exploit something, and for URLs MAX_PATH
> might even be too short, even though the whole code section doesn't make
> sense with URLs and might not even be called.

AFAIK it is used for URLs, too. Though I think that's not a great loss
if it doesn't work for huge URLs, and also instead of crashing it could
print a proper error message. But I think I'll take care of that myself.
Also PATH_MAX on Unix(-like) systems is usually around a page size, or 4kB,
which is plenty even for URLs. Of course Windows as always is a problem
with only 260.
Anyway, patch is applied and thanks for staying on during the review
process.