Re: 2.01.01a>=40: multi.c
Andy Polyakov <appro@fy.chalmers.se> wrote:
> Fix for multi-extent bug in multi.c has [at least] two bugs:
>
> 1. If directory entries constituting multi-extent file end up in
> different sectors read_merging_directory ends up in endless loop.
> Problematic loop is denoted in attached diff along with possible solution.
Coiuld you please explant your claim?
The code you removed is needed and you did not provide a working replacement.
> 2. Given that mkisofs reserves for file system layout generated by
> another iso9660 formatter, alternative partitioning of file to multiple
> extents (for example larger amount of smaller extents) for now would
> result in effective data corruption. Point is that it's not enough to
This is a claim that would need a prove...
> copy extents' isorec.extent fields alone, one has to copy at least
> isorec.size fields as well. As for "at least." *Formally* (i.e. knowing
> nothing about alternative iso9660 formatting and assuming "worst") one
> should copy even isorec.ext_attr_length, [some bits from] flags,
> file_unit_size and interleave. Attached diff denotes problematic code by
> simply copying all of above mentioned fields in single memcpy (along
> with isorec.date field, which is checked to be same as in curr_entry, so
> copying should not cause side effects).
As your patch does not handle the file correctly anymore, we would need to
discuss it.
> Couple of comments not related to above file-system formatting bugs.
>
> There is inconsistency in directory_entry.mxpart declaration and its
> usage. Declaration is "guarded" by #ifdef USE_LARGEFILES, while its
> usage is not. Formally multi-extent files don't have to be "large," i.e.
> "small" files are allowed to be multi-extent too (actually original
> application for multi-extent was *CD* packet writing). Therefore it,
> multi-extent support, should not be associated with USE_LARGEFILES and
> mxpart declaration should not be depend on corresponding macro.
The problem with multi extent files in ISO-9660 is that it allows you to any
kind of nonsense.
> In check_prev_session one can see free(ptr[i]); and in copy_mult_ext one
> can see free(sex);. These are pointers to directory_entry structure,
> which in turn can contain pointers to other dynamically allocated
> objects, most notably rr_attributes and name. Freeing an object without
> freeing other dynamically allocated objects it alone refers to used to
> constitute memory leaks.
>
> As for copy_mult_extent. As mentioned above it copies pieces of
> information from original extent records. As alternative to this field
> copying would it be appropriate to consider simply using original extent
> records, i.e. removing them from orig_contents array and linking them
> into this_dir? A.
>
> P.S. As usual I make no claims on correctness of provided patch, it
> serves primarily educational purposes.
I am currently at OSCON and don't have the time for a closer look.
Let us continue next week.
Jörg
--
EMail:joerg@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
js@cs.tu-berlin.de (uni)
schilling@fokus.fraunhofer.de (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily
Reply to: