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

[Pete Batard]

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.

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.

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


> 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.

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.

Regards,

/Pete