Re: libdvdread: Patch OK?

[Samuel Thibault <sthibault@debian.org>]

Svante Signell, le Sun 04 Sep 2011 23:51:45 +0200, a écrit :
>          }
> -        new_path = malloc(PATH_MAX+1);
> -        if(!new_path) {
> -          goto DVDOpen_error;
> -        }
> -        if( getcwd( new_path, PATH_MAX ) == NULL ) {
> +        if( getcwd( new_path, NULL ) == NULL ) {
>            goto DVDOpen_error;

That can not work: you are not assigning the result of getcwd to
new_path. Also, NULL is not a valid parameter for size, it should be
0. Last but not least, getcwd allocating the area when size is 0 is a
GNU extension, it should be checked with upstream whether that is OK.

>          }
>          retval = fchdir( cdir );
>          close( cdir );
>          cdir = -1;
>          if( retval == -1 ) {
> +          free( new_path );

Mmm, you shouldn't have to add any free() call, since the original
upstream code was already using malloc().

> @@ -635,9 +632,15 @@
>  
>    while( ( ent = readdir( dir ) ) != NULL ) {
>      if( !strcasecmp( ent->d_name, file ) ) {
> +#ifdef __GNU__
> +      asprintf( &filename, "%s%s%s", path,
> +               ( ( path[ strlen( path ) - 1 ] == '/' ) ? "" : "/" ),
> +               ent->d_name );

That won't work: it'll just replace the filename parameter inside the
function, not replace the pointer in the caller. You need to make the
char*filename parameter a char**filename, both in findDirFile and
findDVDFile, and make callers pass &full_path.

> @@ -648,7 +651,11 @@
>  
>  static int findDVDFile( dvd_reader_t *dvd, const char *file, char *filename )
>  {
> +#ifdef __GNU__
> +  char *video_path = NULL;

As already reported by Guillem previously for another package, you
should probably simply use your newer version of the code in all cases.
Special-casing __GNU__ is asking for seeing bugs introduced sooner or
later in the __GNU__ case while other OSes work fine (because tested by
upstream).

Samuel