[syslinux] (no subject)

[Shao Miller]

A nice offering and please kindly accept the following code review. ;)

Gert Hulselmans wrote:
> Examples:
>   chain.c32 fs grub=/boot/grub/stage2 grubcfg=/boot/grub/grub.lst
>   chain.c32 hd1,10 grub=/boot/grub/stage2 grubcfg=/boot/grub/grub.lst

Disadvantage: The user's use of 'grubcfg=...' without an accompanying 
'grub=...' yields no warning/error.  Even if it warned, GRUB might clear 
the screen anyway. :S  Also, a lengthy path will be silently discarded.


> -         grub=<loader>      Load GRUB stage2\n\
> +         grub=<loader>      Load GRUB Legacy stage2\n\
> +         grubcfg=<filename> Set alternative config filename for GRUB
> Legacy\n\

Advantage: This keeps the usage narrow. :)

> -	    /* GRUB's stage2 wants the partition number in the install_partition
> +	    /*
> +	     * GRUB's stage2 wants the partition number in the install_partition
>  	     * variable, located at memory address 0x8208.
> -	     * We only need to change the value of memory address 0x820a too:
> -	     *   -1:   whole drive (default)
> +	     *
> +	     * It looks very similar to the "boot information format" of the
> +	     * Multiboot specification:
> ...

Some nice documentation here for the curious. :)

> +	    if (opt.grubcfg && strlen(opt.grubcfg) <= 88
> +		&& data[ndata].size > 0x26f)
> +
> +		memcpy((char *)data[ndata].data + 0x217, opt.grubcfg,
> +		       strlen(opt.grubcfg) + 1);
>  	}

Even so, some of these numbers are mystical and might be better off as 
'#define's or 'enum { ... };'s.  The documentation certainly helps.

For your '88': Personally, I enjoy using an array of 'char xxx[89];' 
somewhere and then using 'sizeof(xxx)' or 'sizeof(xxx) - 1' wherever I 
need to talk about its size.  In this case, one way to do that would be 
to put the array in a struct and have a pointer-to-that-struct pointed 
at this special region in the stage2 binary.

Quite a co-incidence of timing where Paul Bolle submitted a similar 
patch nearly simultaneously!  It would be really great if chain.c32 got 
the best of both. :)

I don't speak on behalf of Syslinux; just my review.

- Shao Miller