[Bug 601125] [NEW] code maintainability, modularity and readability issues

Stuart R Balfour sbalfour at att.net
Fri Jul 2 17:27:54 BST 2010

Public bug reported:

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 

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

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

        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.

** Affects: dvd+rw-tools (Ubuntu)
     Importance: Undecided
         Status: New

code maintainability, modularity and readability issues
You received this bug notification because you are a member of Ubuntu
Burning Team, which is subscribed to dvd+rw-tools in ubuntu.

More information about the Ubuntu-burning mailing list