[k3b] [Bug 367639] k3b fails to continue multisession (blue ray)

[Thomas Schmitt via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=367639

--- Comment #38 from Thomas Schmitt <scdbackup at gmx.net> ---
Hi,

Leslie Zhai wrote:
> Hi Thomas, please review it ;-)

Most problematic:

I do not see where you hand over the growisofs-ly computed -C values
to mkisofs. It could be that it gets to see -C 0,0 despite the effort
in k3bgrowisofswriter.cpp.

I am uncertain whether i caught all problems. You need to test with
(maybe emulated) DVD+RW medium. (Not DVD+R or DVD-R.)

-------------------------------------------------------------------------

> http://commits.kde.org/k3b/9a6340ed76aaa8fa8784168ef7d286710097cee3

Looks good to me, under the assumption that d->multiSessionInfo
contains the suitable values which growisofs will accept.

-------------------------------------------------------------------------

> http://commits.kde.org/k3b/b22f60344db146aa6b5136373b0a0b270d5d8ee9

I riddle about this gesture

+                if (d->image.isEmpty())
+                    fptr = fopen("/dev/fd/0", "r");

It is clearly wrong to open "/dev/fd/0" out of multiple reasons:

- It is unclear to what the stdin of K3B is connected.
  Quite surely not to the stdout of the mkisofs run.
  And if so, then reading would consume irrevocably the start of the
  ISO stream. This is a show stopper !

- "/dev/fd/0" is both, a Linux device address and a symbolic address
  in growisofs. If growisofs sees this address, then it does not open
  the Linux file but rather uses stdin (the already open file descriptor 0).
  See growisofs.c:
                        if (sscanf(in_image,"/dev/fd/%u",&imgfd) == 1)
                            imgfd = dup (imgfd); /* validate descriptor */

- stdin is not seekable.
  fseek(3) will probably cause errno EBADF.

Under what circumstance is the condition d->image.isEmpty() true ?
Can this happen at all, when d->multiSession is true ?

------

+ fptr = fopen(d->image.toStdString().c_str(), "r");

Is it sure that this path leads to the random-access readable file object
which hold the previous ISO 9660 session ?

------

You do not check for the ISO 9660 magic number before you read the size.
This is absolutly mandatory or else the pseudo Next Writable Address will
be more or less random and growisofs will most probably not accept it.

Roughly (needs error handling with fseek and fread):

     char buf[6];
     fseek(fptr, 32 * 1024, SEEK_SET);
     fread(fptr, 1, 6, buf);
     if (buf[0] != 0x01 || buf[1] != 'C' || buf[2] != 'D' ||
         buf[3] != '0'  || buf[4] != '0' || buf[5] != '1') {

         // TODO: handle that no ISO 9660 is present at medium start

     }

------

+                        fread(buf, 1, sizeof(buf), fptr);
+                        d->process << "-C 0," << buf;

Between these two line you have to convert the 4 bytes into a 32 bit
number by interpreting them as little-endian number.
Your code would fail on machines with big-endian byte order.
(They are rare but still exist.)

Further you did not round up to full multiples of 16.

I'd do (in the hope that i understand the "d->process << " gesture):

                         uint32_t c2;
                         uint8_t buf[4];

                         ret = fread(buf, 1, sizeof(buf), fptr);
                         if (ret != sizeof(buf)) {
                            // TODO: handle inability to read ISO size
                         }

                         // Interpret the read bytes as little-endian number.
                         // See also growisofs.c function from_733().
                         c2 = buf[0] | (buf[1] << 8) | (buf[2] << 16) |
                              (buf[3] << 24);

                         // Round up to full multipes of 16, as growisof does
                         // in its function setup_C_parm().
                         c2 += 15;
                         c2 /= 16;
                         c2 *= 16;

                         d->process << "-C 0," << c2;

uint32_t and uint8_t from C header <stdint.h> guarantee the size and
unsignedness.
If they are not acceptable to K3B, use "unsigned long" and "unsigned char".

(Note that "<<" has two completely different meanings here.
 A C++ promoter who ridicules BASIC's GOTO should be asked to compare
 it with the inheritance piles and spaghetti overloading of C++.)

------

+                        qWarning() << strerror(errno);

Warning seems to weak as reaction to me.
We noticed an unusable d->multiSessionInfo and try to replace it by
a usable one. Now this failed and we actually have to refuse the burn
run, because it would fail with a growisofs error message.

Also, if you just show the error message, then the user will not know
why you tried to fseek and read.
I'd throw a severe error and report
  "Medium is not of multi-session type and does not contain ISO 9660.
   Cannot emulate multi-session on it."

-------------------------------------------------------------------------

> http://commits.kde.org/k3b/e9faad4a24e80ee83a3881707e394ca4be66f5c7

I understand this is a correction to the previous patch.
(Did "<<" buf show undesired bytes ?)

+                        int next = atoi(buf);

The signed data type "int" is slightly wrong here. The problem would
show up only with ISOs of size 4 TiB or larger. Nevertheless, the field
is defined by ECMA-119 as unsigned 32 bit stored in both endiannesses.

atoi(buf) will yield wrong results on big-endian machines. You first need
to left-shift the bytes into their little-endian positions before you can
treat the result as 32 bit number.

                         // Interpret the read bytes as little-endian number
                         c2 = buf[0] | (buf[1] << 8) | (buf[2] << 16) |
                              (buf[3] << 24);

Further atoi(3) yields signed int. (Again, a problem only with >= 4 TiB.)

--------------------------------------------------------------------------

We are getting nearer to the goal.

I am undecided whether interpretation of ISO size is correctly located
in k3bgrowisofswriter.cpp .

--------------------------------------------------------------------
Pro:

You can follow the habits of growisofs as awkward they may be.

Contra:

You must give mkisofs the same -C values as you tell growisofs.
So if you leave the computation in k3bgrowisofswriter.cpp then you
must update d->multiSessionInfo and you must make sure that the
mkisofs run is composed only after d->multiSessionInfo was corrected.

Multi-session emulation on overwritable media is not only provided by
growisofs. So if other capable programs get used by K3B, one will have
to duplicate large parts of the ISO inspection.
(A habit to align to larger granularity than 16 could possibly be handled
 in the specialized writer by rounding again. This works only if the
 new alignement is a multiple of 16.)

--------------------------------------------------------------------

So i'd imagine this architecture:

- A generic checker for d->multiSessionInfo, which tries to determine
  a better value from ISO if the current value is 0,0.
  This function would _not_ round the c2 value.

- A writer specific function which inspects the c2 value and rounds it
  to the granularity which the writer program uses. (16 for growisofs.)

- Only after this is done you may compose the program arguments of
  mkisofs and writer program.

--------------------------------------------------------------------

Have a nice day :)

Thomas

-- 
You are receiving this mail because:
You are the assignee for the bug.