Re: [Libcdio-devel] [RFC] New API iso9660_statv2_t as API/ABI compatible way to read files >= 4 GiB

[Pete Batard]

Hi Thomas,

On 2018.07.06 19:09, Thomas Schmitt wrote:
> i have pushed the very large commit 87f5053 to branch ts-multiextent :

Wow! Indeed.

Just for the record, I'm seeing a couple of warnings when compiling with 
MinGW-w64 on Windows, but the tests look all good.

Here they are:
-------------------------------------------------------------------------
make[3]: Entering directory '/d/libcdio/lib/iso9660'
  CC       iso9660.lo
  CC       iso9660_fs.lo
  CC       rock.lo
  CC       xa.lo
iso9660_fs.c: In function '_iso9660_statbuf_alloc_extents':
iso9660_fs.c:866:37: warning: format '%lu' expects argument of type 
'long unsigned int', but argument 2 has type 'long long unsigned int' 
[-Wformat=]
     cdio_warn("Could not allocate %lu bytes",
                                   ~~^
                                   %I64u
              (unsigned long) num_extents * 
sizeof(iso9660_extent_descr_t));
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
iso9660_fs.c: In function '_iso9660_dir_to_statbuf':
iso9660_fs.c:914:23: warning: array subscript is above array bounds 
[-Warray-bounds]
   p_stat->magic_number[1] = 2;
   ~~~~~~~~~~~~~~~~~~~~^~~
  CCLD     libiso9660.la
make[3]: Leaving directory '/d/libcdio/lib/iso9660'
-------------------------------------------------------------------------

> I see only two problems with his proposal:
>
> * It breaks the ABI.
>
> * It will break the ABI again if ever the maximum number of extents
>    ISO_MAX_MULTIEXTENT shall be incresed from 8 to a higher number.
>    It is unattractive to choose a large value already now, because each
>    extent slot costs 8 byte and iso9660_stat_t can have many instances.
>
>    (This could be fixed without a new API by cherrypicking parts of my
>     proposal. I will probably do, if iso9660_statv2_t gets rejected.)

And I am still going to maintain that, empirically, we are going to find 
that both these issues, and especially the second one, are overblown.

My reasoning for that is that ABI breakage is something that happens all 
the time, and that any sensible developer/OS should be able to handle in 
their stride at this stage. Not to say that this is not an 
inconvenience, but I think we should indeed qualify it for what it is: 
an inconvenience rather than a problem. Also, as I stated before, 
considering that we recently release libcdio 2.0, I don't personally 
have much of an issue if we want to sit on ABI-breakine multiextent 
changes for a year or two, to give some breathing space for developers 
and OS/package maintainers who just carried out the 2.0 upgrade.

With regards to point #2, I am basing my _anticipation_ on the following:
I am the developer of a relatively successful libcdio-based application, 
that has seen steady multi-million monthly downloads for the past few 
years, and to which people are throwing all kind of ISOs. Yet, it has 
only been until a few months ago that I came come across a report from 
someone who was using a multiextent ISO, with a single extent.

Considering that (as my mailbox tends to demonstrate) users of my 
application are not especially shy about letting me know when something 
doesn't work as expected, it gives me some relatively sound basis to 
think that we are _exceedingly_ unlikely to come across non-academical 
ISO-9660 images that are going to use more than 8 extents in the future 
and that, at best, all the work we (or, more accurately: Thomas) have 
been doing is probably going to benefit less than a couple of people.

So, to me, it looks like a lot of effort to help a handful of people who 
most likely would just have to recompile whatever app they are using to 
use a static libcdio library where they just change the hardcoded limit, 
according to their need, and, as such, it seems like a wasted effort...

Besides time constraints, this is also the reason why I preferred to 
bail out, than commit to addressing what I consider to be unlikely 
actual issues.

> It turned out that avoiding thee two ABI problems implies to stirr up
> a lot of code.
> Pete is officially allowed to laugh now.

Seeing the extent of your changes, and the point that I am kind of 
making against them, I really don't feel like laughing.

I have been in the same situation as you many times over, with a 
changeset proposal that I put a lot of effort in (and that I would also 
have no trouble to argue was technically brilliant), but that got shot 
down by the original project for various reasons.

So, I do feel bad about currently leaning against your proposal, on 
account that, considering that the idea is that libcdio users will 
ultimately upgrade to using v2, that code upgrade to v2 looks to be a 
lot more painful than simply being able to reuse a reworked current 
struct, even if that results in ABI breakage. Sorting out a broken ABI 
is a common issue for which you can find a lot of help. Sorting out the 
use of a newly introduced custom struct is a different matter. So, as a 
developer, my feeling is that, give the choice, I'd rather go through an 
ABI breakage.

But of course, I'm a bit too close to one side of the debate to be 
considered objective on the matter, so please feel free to take what I 
am saying with a grain of salt. This is even more more true as, in my 
application, I am statically linking my own custom version of libcdio, 
so I don't really care much abut API/ABI breakage...


At any rate, since I have now somewhat made a case against the 
introduction of these changes, I certainly wouldn't mind seeing other 
people chiming in, who instead might offer some good reasons to want to 
see these changes in.

After all, now that we have this massive effort, it would certainly feel 
like kind of a waste not to use it...

Regards,

/Pete