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

[Rocky Bernstein]

On Mon, May 25, 2020 at 6:42 AM Pete Batard <address@hidden> wrote:

> Hi,
>
> Thanks to you both for following up on multiextent. I realize I probably
> should have spent a bit more time finalizing the proposal, as there were
> indeed a few missing items.
>
> On 2020.05.25 10:41, Rocky Bernstein wrote:
> > On Mon, May 25, 2020 at 4:23 AM Thomas Schmitt <address@hidden>
> wrote:
> >> 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?
>
> Well, I think the proposed bump to 3.0 with the update of NEWS.md, as
> well as an official announcement on this list should do it.
>
> *However*, I would like us to wait with the 3.0 release, as I am working
> on adding Rock Ridge Deep Directory support, and this may require
> further API changes.
>


No problem. There's no rush so take your time.

As Bob Dylan said. "We've been a long time coming, and an even longer time
gone."


>
> Internally, I actually have a working RR Deep Directory solution in
> Rufus, but it requires a bit too much awareness of the intrinsics of
> deep directories by the calling application to my liking.
>
> For reference, this is my current diff for the API (not finalized):
>
>
> -----------------------------------------------------------------------------
> diff --git a/src/libcdio/cdio/iso9660.h b/src/libcdio/cdio/iso9660.h
> index 0430890..a0dff90 100644
> --- a/src/libcdio/cdio/iso9660.h
> +++ b/src/libcdio/cdio/iso9660.h
> @@ -565,6 +565,8 @@ struct iso9660_stat_s { /* big endian!! */
>     iso9660_xa_t       xa;              /**< XA attributes */
>     enum { _STAT_FILE = 1, _STAT_DIR = 2 } type;
>     bool               b_xa;
> +  struct _iso9660_s* p_iso;
> +  char*              dirname;
>     char               filename[EMPTY_ARRAY_SIZE];    /**< filename */
>   };
>
> @@ -582,7 +584,8 @@ extern enum iso_extension_enum_s {
>     ISO_EXTENSION_JOLIET_LEVEL2 = 0x02,
>     ISO_EXTENSION_JOLIET_LEVEL3 = 0x04,
>     ISO_EXTENSION_ROCK_RIDGE    = 0x08,
> -  ISO_EXTENSION_HIGH_SIERRA   = 0x10
> +  ISO_EXTENSION_HIGH_SIERRA   = 0x10,
> +  ISO_EXTENSION_ROCK_RIDGE_DD = 0x20
>   } iso_extension_enums;
>
>
> @@ -967,6 +970,10 @@ iso9660_stat_t
> *iso9660_ifs_find_lsn_with_path(iso9660_t *p_iso,
>                                                  lsn_t i_lsn,
>                                                  /*out*/ char **ppsz_path);
>
> +void iso9660_enable_rr_dd(iso9660_t* p_iso);
> +void iso9660_disable_rr_dd(iso9660_t* p_iso);
> +bool iso9660_is_rr_dd_enabled(iso9660_t* p_iso);
> +
>   /*!
>     Free the passed iso9660_stat_t structure.
>
> diff --git a/src/libcdio/cdio/rock.h b/src/libcdio/cdio/rock.h
> index 8ae6680..38609be 100644
> --- a/src/libcdio/cdio/rock.h
> +++ b/src/libcdio/cdio/rock.h
> @@ -190,12 +190,12 @@ typedef struct iso_rock_nm_s {
>
>   /*! Child link. See Section 4.1.5.1 */
>   typedef struct iso_rock_cl_s {
> -  char location[1];
> +  iso733_t location;
>   } GNUC_PACKED iso_rock_cl_t ;
>
>   /*! Parent link. See Section 4.1.5.2 */
>   typedef struct iso_rock_pl_s {
> -  char location[1];
> +  iso733_t location;
>   } GNUC_PACKED iso_rock_pl_t ;
>
>   /*! These are the bits and their meanings for flags in the TF
> structure. */
> @@ -279,6 +279,11 @@ typedef struct iso_rock_time_s {
>     } t;
>   } GNUC_PACKED iso_rock_time_t;
>
> +/* Attributes used for deep directory support */
> +#define RR_DD_CL 0x01
> +#define RR_DD_PL 0x02
> +#define RR_DD_RE 0x03
> +
>   typedef struct iso_rock_statbuf_s {
>     bool_3way_t   b3_rock;              /**< has Rock Ridge extension.
>                                            If "yep", then the fields
> @@ -309,13 +314,17 @@ typedef struct iso_rock_statbuf_s {
>     uint32_t i_rdev;                    /**< the upper 16-bits is major
> device
>                                            number, the lower 16-bits is the
>                                            minor device number */
> +  uint8_t       dd_attributes;        /**< Deep Directory attributes */
>
>   } iso_rock_statbuf_t;
>
>   PRAGMA_END_PACKED
>
>   /*! return length of name field; 0: not found, -1: to be ignored */
> -int get_rock_ridge_filename(iso9660_dir_t * de, /*out*/ char * retname,
> +int get_rock_ridge_filename(iso9660_dir_t * de,
> +                            /*in*/ const char * psz_dirname,
> +                            /*in*/ void * p_iso,
> +                            /*out*/ char * retname,
>                               /*out*/ iso9660_stat_t *p_stat);
>
>     int parse_rock_ridge_stat(iso9660_dir_t *de, /*out*/ iso9660_stat_t
> *p_stat);
>
> -----------------------------------------------------------------------------
>
> I'm in the process of seeing if I can come up with something better and
> then creating a new rock-ridge-deep-directory branch in libcdio for review.


Ok.


>


> So I would advise against being too hasty with the 3.0 release.
>

Ack.


>
>
> > 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.
>
> I think I would rather have something like:
>
> This version adds multi-extent support for ISO-9660. Previously,
> additional extents were treated as separate files (in the manner they
> are stored internally) instead of the continuation of a previous file,
> which also meant that libcdio was unable to handle ISO-9660 files that
> were larger than 4 GB, i.e. the size limit of a single extent.
> In this version, libcdio recognizes extents and treats them as belonging
> to a single larger file, as long as the blocks making up an ISO-9660
> file are strictly sequential. If the block are not sequential, a warning
> message is generated and the `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.
>
> I would drop these 2 lines.
>
> >> 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
>
> ^ Typo on "attribute" and I think we should mention that it is 64-bit like:
>
> * switch from using `iso9660_stat_t.size` to the new 64-bit attribute
>
> >> `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
>
> ^ Typo on language
>
> >> `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.
>

Ok. Would you please just commit these changes in the branch. If I do I'll
just make more mistakes.

Thanks.

Then @Thomas Schmitt <address@hidden>  would you review what Pete has.
Thanks, both.


> Looks good. And the other proposed changes from
> 'revise-examples-for-multi-extent' look good to me too.
>
> Depending on how I fare coming up with an improved solution, I'll try to
> create the a Rock Ridge Deep Directory branch some time this week, so
> that hopefully we can make this part of the 3.0 release as well.
>

Sure, if we're going to disrupt may as well go as far a we can.



>
> Regards,
>
> /Pete
>
>