code maintainability, modularity and readability issues

[Stuart R Balfour]

Binary package hint: dvd+rw-tools

dvd+rw-tools-7.1-6, kubuntu 10.04

The developer of this package is apparently no longer active. The source
appears to have been cobbled together piecemeal over time, and now
needs a redraft for style and maintainability if others are to take over
work.

Specific suggestions:

1. there is much duplicate code in the *_format and *_finalize set of
functions in growisofs_mmc.cpp and each of these sets ought to be
combined into a single format/finalize function.

2. fetching and storing 4-byte integers from and into byte streams with
code like the following:

        cmd[2] = (lba>>24)&0xff;
        cmd[3] = (lba>>16)&0xff;
        cmd[4] = (lba>>8)&0xff;
        cmd[5] = lba&0xff;

        next_wr_addr = track[12]<<24;
        next_wr_addr |= track[13]<<16;
        next_wr_addr |= track[14]<<8;
        next_wr_addr |= track[15];

should be replaced with calls to inline functions putint and getint.
Linux implementations already define functions to load and store
32-bit words converting among endian byte-orders.

3. The SCSI related codes (constants) for commands, formats, modes,
etc ought to be collected into enums, and all inline instances replaced
with the enum id.

4. cryptic pieces of code, especially bit-manipulation, ought to be simplified
or replaced with calls to library functions or user-defined functions. For example
(line 1639 of growisofs_mmc.cpp):

    blocks = (unsigned int)(size/2048);
    blocks += 15, blocks &= ~15;
    blocks /= 16;
    blocks += 1;
    blocks /= 2;
    blocks *= 16;

The semantics are: "set 'blocks' to half the number of 2K blocks in 'size',
rounded up to a multiple of 16". Say what you mean:

#define round_up(n,m) round_(n,m,1)
#define round_dn(n,m) round_(n,m,-1)
#define round(n,m) round_(n,m,0)
#define round_(n,m,mode) ( (mode>0) ? ((n+m-1)/m*m) \ // round n up to multiple of m
                                            : (mode<0)? (n/m*m) \ // round n down to multiple of m
                                            : ((n+m/2)/m*m) ) // (mode=0) round n to nearest multiple of m
blocks =round_up( (unsigned int)(size/2048)/2, 16);

The code is now one line instead of six, and a very useful function
has been defined (which ought to be moved to a common header file).
The original code also has a subtle problem, which the clear code does
not.

In addition, everywhere where rounding to 16 occurs, in expressions like:

        blocks += 15, blocks &= ~15;

the rounding occurs because DVD block size is 32K or 16*2048 byte blocks
see this define at line 562 of growisofs.c:

#define DVD_BLOCK (32*1024)

Note: DVD_BLOCK should really be written as (16*2048) or (16*CD_BLOCK)

Those 15's really represent (DVD_BLOCK/2048-1) and ought to be represented that way
so we have a way of finding all the code dependencies related to DVD block size (i.e.,
what happens when we go to 64K block size for BD-R? see bug 601092).

Similarly, expressions like "x<<11" represent "x*2048". Arguably, all instances of
2048 ought to be replaced with CD_BLOCK (defined as 2048 at line 560 of growisofs.c).

There are numerous other places where functions like min,max,abs,
round/truncate and other bit-manipulations are being performed in-line
in the code that ought to be replaced with common function calls. Some
of those instances contain bugs, because the code is unclear and tricky.

[Schily (schilling-fokus)]

It seems that you missunderstand two constraints:

- There is no development in Ubuntu, so proposing source changes here are unlikely to have any affect.

- Applying the changes you propose would result in giving up portability.

[Stuart R Balfour (sbalfour)]

Yes. I understand. "Debian" appears to be the nominal owner of the package,
though no one at debian or anywhere else is maintaining it. In such circumstances,
Ubuntu release teams sometimes elect to fix and distribute their own packages.
No work has been done on this package since at least 2008, and that's an eternity.

I don't believe my changes, properly conditionalized, introduce portability changes.
Many compilers and/or implementation libraries have functions to convert among
endian byte orders, or at least to fetch serially ordered bytes in multiples of 2,4 or 8
from byte streams and assemble them into words and double words. That's the only
change I identify as having portability implications. This package is fundamentally a GNU/
C/Linux program. To whatever extent an upgrade would enable some subset of the
existing user base to enjoy increased feature content, reliability and currectness,
that is a plus. Right now, this package is dead.