Re: Enhancing cdboot [patch for review]

On Monday 08 December 2008 06:51:19 pm Luigi Rizzo wrote:
> On Mon, Dec 08, 2008 at 02:40:41PM -0800, Maxim Sobolev wrote:
> > Hi,
> > 
> > Below please find patch that enhances cdboot with two compile-time 
options:
> ...
> > Any comments/suggestions are appreciated. If there are no objections I
> > would like to commit the change. The long-term goal is to make
> > CDBOOT_PROMPT default mode for installation CD.
> > 
> > http://sobomax.sippysoft.com/~sobomax/cdboot.diff
> 
> Looks good. Some comments:
> 1. since there is plenty of space in the cdboot sector, why don't you
>    make the two option always compiled in, controlling which one to
>    activate through flags in the bootsector itself, to be set
>    patching the binary sector itself using a mechanism similar to
>    boot0cfg.
>       Of course you cannot alter a cdrom after you burn it,
>    but it makes it easier to build CDs with one or the other defaults,
>    patching cdboot or the iso image itself before creating/burning it.

I don't think this is very useful because CDs are read-only.  You can just as 
easily build a different cdboot rather than having to write some custom 
cdbootcfg util to patch the binary.

> 2. in fact, the 'silent' option could be disabled at runtime by
>    pressing some key (e.g. adding a short wait loop before proceeding;
>    if this is meant for custom, unattended CDs the extra delay should not
>    matter much);

I don't imagine anyone will know to press a key to get verbose messages, and 
the CD boot process is quick enough you would have to add an artificial delay 
to it to allow for the keypress.

> 3. one nitpick -- in one of the first chunks you replace $start
>    with $LOAD, but if i am not mistaken operation depends on $LOAD = $start,
>    so why don't you always use the same ?

No, because he relocates it, $start is now the relocated address, but the BIOS 
loads it at LOAD which is now != $start.

>      Also in terms of relocation size, wouldn't it be the case of
>    hardwiring the size of the cd boot sector:
> 
> -       mov $((end_init - start)/2),%cx
> +       mov 1024,%cx

I prefer the existing code to make sure and copy the full boot loader, 
whatever it's size is.

Maxim,

My only comment is to please make the new block comment match the style of the 
existing block comments by having '#\n' lines before and after.

-- 
John Baldwin