Re: [Libcdio-devel] Hindsight is 2020 - Let's sort multiextents at last

[Rocky Bernstein]

On Mon, May 25, 2020 at 4:23 AM Thomas Schmitt <address@hidden> wrote:

> Hi,
>
> the changes in revise-examples-for-multi-extent look good to me.
>
> But there might be some more occasions of ->size which need change.
> I did:
>   fgrep -r '>size' . | less
> and tried to classify the found code snippets.
>

Thanks for looking over. Indeed as often the case with my changes, there is
much more to be desired.



>
> Please review.
>
> Further we need to think about proper announcement of the change.
> I quote some older text of me for mining at the end of this mail.
>

Ok. I don't know what the best way to do this is though. Suggestions?



>
>
> -----------------------------------------------------------------------------
> Occasions where i think that ->total_size should be used:
>
> -------------------------
> ./example/C++/isofile.cpp:
>   if (ftruncate (fileno (p_outfd), p_statbuf->size))
>

Should be fixed in commit  d10f3a4d

>
> p_statbuf comes from iso9660_ifs_stat_translate().
> It is used in the same (main) function as
>    CEILING(p_statbuf->total_size, ISO_BLOCKSIZE)
>
> So i expect that the ftruncate() call should get ->total_size too.
>

Right. Should be fixed in commit  d10f3a4d

>
> --------------------------
> ./example/C++/OO/iso4.cpp:
>   p_s->p_stat->lsn, p_s->p_stat->size, psz_path, filename);
>
> Here i stand somewhat in the woods because p_s is obtained by some C++
> specific iteration. But its ->p_stat seems not to be a stat(2) struct.
> So i assume that ->total_size would be appropriate.
>

 Changed in d10f3a4d. Seems to work for now ;-)


> -------------------
> ./doc/libcdio.texi:
>   for (i = 0; i < p_statbuf->size; i += ISO_BLOCKSIZE)
>
> Not active code, but clearly in need of modernization.
>

Have revised in  d10f3a4d. Thanks for catching.

>
>
>
> -----------------------------------------------------------------------------
> fgrep matches which to my opinion need no change:
>
> ./lib/driver/image/nrg.c:
>   env->size = MAX (env->size, (start_lsn + sec_count));
>   ... and more of these ...
> env is _img_private_t.
>
> ./lib/iso9660/iso9660.c:
>   uint32_t dsize = from_733(idr->size);
>   ... and more of these ...
> idr is iso9660_dir_t.
>
> ./lib/iso9660/iso9660_fs.c:
>   p_stat->total_size += from_733(p_iso9660_dir->size);
>   p_stat->size = from_733(p_iso9660_dir->size);
>   p_stat->secsize = CDIO_EXTENT_BLOCKS(p_stat->size);
> These are part of the augmentation for multi-extent and thus appropriate.
>
> =========================================================================
>
> The old applications would still work for single-extent files.
> But libcdio should announce the new ->total_size quite loudly.
>

Ok. Not sure where to go. The usual thing is to add this to NEWS.md


>
> Maybe one could strip down my description from summer 2018:
> -------------------------------------------------------------------------
>
> pragmatic-multiextent [...]
> can create iso9660_stat_t objects only from files of which the
> extents form a sequentially readable byte string.
>
> If this expectation is not fulfilled, then pragmatic-multiextent issues
> a warning message and does not create an iso9660_stat_t object for the
> affected file. I.e. the file will not appear in readdir()-ish results and
> individual inquiries will yield NULL rather than an iso9660_stat_t object.
>
> mkisofs and libisofs currently only produce the expected simple extent
> layout. Up to now, no other kind of multi-extent files have been found
> in the wild. (I had to fake a test ISO with unaligned extent size.
> An ISO where extents hop forth and back has still to be counterfeit.)
>
> [... omit statements about ABI breakage and regression probability ...]
>
> [Instead, one should mention:
>  The old libcdio applications will still work for single-extent files.
>  But if they shall handle large multi-extent files they need to be
>  adapted:
> ]
>
> Applications only have to switch from using
>   iso9660_stat_t.size
> to using new member
>   iso9660_stat_t.total_size
> and to make sure that their loop controlling variables can cope with
> integer numbers up to 43 bit (32 bit block address, 11 bit byte address in
> block).
>
>
In commit  32785270 I added this:

version 3.0.0
> =============
> This version adds multi-extent support for ISO-9660. Previously the blocks
> making up an ISO-9660 file had to come strictly sequentially.
> In this version, when libiso9600 detects that this is not the case, a
> warning message is generated and a `iso9660_stat_t` object is not created.
> the file will not appear in `readdir()`-ish results and individual
> inquiries will yield `NULL` rather than an `iso9660_stat_t` object.
> Programs written for previous versions of _libcdio_ will still work for
> single-extent files. However in order to handle large multi-extent files,
> applications
> need to be adapted:
> * switch from using `iso9660_stat_t.size` to the new attrtibute
> `iso9660_stat_t.total_size`
> * make sure that loop controlling variables can cope with integer numbers
> up to 43 bit numbers. In C-like lanaguage this may mean switching from
> `int` to `long int`
> Notes: 43 bits is the sum of a 32-bit block address plus an 11-bit byte
> address in a block); `mkisofs` and `libisofs` currently only produce the
> files with sequential blocks.



Feel free to revise or comment on any of the changes just made in that
branch. It's okay just to directly commit in that branch.

-------------------------------------------------------------------------
>
> Have a nice day :)
>
> Thomas
>
>
>