Re: [Libcdio-devel] Rock Ridge deep directory support

[Pete Batard]

Hi Thomas,

I'm happy to see you are doing better now. :)

On 2020.06.13 13:16, Thomas Schmitt wrote:
> just to show that i began to review the Rock Ridge Deep Directory code,

Thanks!

> i wonder why the RR-related code snippet in lib/iso9660/iso9660_fs.c
> line 832 is not surrounded by #ifdef HAVE_ROCK and how a freshly calloc'ed
> p_stat shall have (p_stat->rr.u_su_fields & ISO_ROCK_SUF_RE) true.
>
> Further i wonder whether the ISO_ROCK_SUF_RE bit can only be set if
> _iso9660_is_rock_ridge_enabled(p_image).
>
> Line 819:
>
>    /* Reuse multiextent p_stat if not NULL */
>    if (!p_stat) {
>      p_stat = calloc(1, stat_len);
>      first_extent = true;
>
> > > > how could p_stat->rr.u_su_fields become non-zero ?

Technically we could have a non NULL last_p_stat and with p_stat = 
last_p_stat the calloc is not guaranteed and p_stat->rr.u_su_fields 
being zero is not guaranteed...

>    } else {
>      first_extent = false;
>    }
>    if (!p_stat) {
>      cdio_warn("Couldn't calloc(1, %d)", stat_len);
>      return NULL;
>    }
>    p_stat->type    = (p_iso9660_dir->file_flags & ISO_DIRECTORY)
>      ? _STAT_DIR : _STAT_FILE;
>
> > > > why no: #ifdef HAVE_ROCK ?

Because it's redundant. The ISO_ROCK_SUF_RE flag will be set to a non 
zero only if HAVE_ROCK is enabled (because this is only done in 
get_rock_ridge_filename() which is guarded), so there's no point in 
adding the guard where it is not needed, and there is really no 
performance hit by performing a simple test like this one always.

> Line 832:
>
>    /* Ignore Rock Ridge Deep Directory RE entries */
>
> > > > why no: if (_iso9660_is_rock_ridge_enabled(p_image)) ?

Because this is performed in get_rock_ridge_filename() and 
ISO_ROCK_SUF_RE can only be set if that is the case.

I explicitly designed the code around the idea that the flags we test 
can only be set if:
- HAVE_ROCK is defined
- _iso9660_is_rock_ridge_enabled() is true
so that we don't have to re-check for the above when checking the flags.

Regards,

/Pete

>    if (p_stat->rr.u_su_fields & ISO_ROCK_SUF_RE)
>      goto fail;
>
> > > > why no: #endif ?
>
>
> I will continue to read in a few hours.
>
>
> Have a nice day :)
>
> Thomas