Bug#345931: A patch closing #345931, verified against debian testing

[Otavio Salvador]

Mats Erik Andersson <ynglingatal at yahoo.se> writes:

>  Yesterday I fetched from Debian Sid grub 0.97-16.1,
>  built it and observed that, as expected according
>  to #345931, it destroyed and made useless the MBR
>  of my grub 0.95+cvs20040624-17sarge1. But by an
>  application of my patch the altered version of 
>  of grub 0.97 (the patch does not touch any code
>  alterations of debian) correctly is able to
>  reactivate the the bootloader installed by grub 0.95.

I agree that compatibility here is important.

>  Let me stress that the patch simply make shore that
>  the relevant technique used for boot-drive-workaround
>  of grub 0.94-0.97 is respected by grub 0.97. The
>  revision 0.97 originally disregards this backwards
>  compatibility. At the end of the patch, there is
>  an indication of how compatability may be extended
>  to versions 0.92-0.93, should the maintainers so
>  desire.

Ok.

>  Lastly, Robert Millan asks for relevance visavi
>  Grub2. A quick check reveals that grub 1.94 does
>  indeed utilize exactly the same workaround for
>  harddisk booting as grub 0.97 does. The position
>  at which this workaround is checked and applied is
>
>     grub-1.94/util/i386/pc/grub-setup.c  line  250.
>
>  That position is the place to apply a patch with
>  code accomodations to the new settings.
>
>  HOWEVER, it is not clear to me if the Maintainers
>  desire compatibility of Grub2 backwards to Grub
>  Legacy in the sense that the former should be able to 
>  reactivate a bootloader installed by the latter.
>  The maintainers need to decide on this on their own.
>  I have not delved into the complete code of Grub2.

I think you should contact upstream and check if they wants to have
compatibility and then provide a patch to them.

Let's now review your current patch:

> diff -Naur grub-0.97-16.1.old/stage2/builtins.c grub-0.97-16.1/stage2/builtins.c
> --- grub-0.97-16.1.old/stage2/builtins.c	2006-09-27 22:48:46.068364160 +0200
> +++ grub-0.97-16.1/stage2/builtins.c	2006-09-28 00:15:47.535580488 +0200
> @@ -2229,13 +2229,31 @@
>    *((unsigned char *) (stage1_buffer + STAGE1_FORCE_LBA)) = is_force_lba;
>  
>    /* If DEST_DRIVE is a hard disk, enable the workaround, which is
> -     for buggy BIOSes which don't pass boot drive correctly. Instead,
> -     they pass 0x00 or 0x01 even when booted from 0x80.  */
> +   * for buggy BIOSes which don't pass boot drive correctly. Instead,
> +   * they pass 0x00 or 0x01 even when booted from 0x80.                 */

I agree that your proposed style is better but we would like to reduce
code divergence so please, revert this one since this is just code
style fixing.

>    if (dest_drive & BIOS_FLAG_FIXED_DISK)
> -    /* Replace the jmp (2 bytes) with double nop's.  */
> -    *((unsigned short *) (stage1_buffer + STAGE1_BOOT_DRIVE_CHECK))
> -      = 0x9090;
> -  
> +    {
> +    if ( *((unsigned char *) (stage1_buffer + STAGE1_BOOT_DRIVE_CHECK)) == 0xeb )
> +	    /* For present version 0.97: 
> +	     * Replace the jmp (2 bytes) with double nop's.  */
> +       *((unsigned short *) (stage1_buffer + STAGE1_BOOT_DRIVE_CHECK))
> +         = 0x9090;
> +    else if ( *((unsigned char *) (stage1_buffer + STAGE1_BOOT_DRIVE_CHECK)) == 0x80 )
> +         {   
> +	    /* For previous versions 0.94-96:
> +	     *  Set the "boot drive mask". This is the old workaround for buggy BIOSes 
> +	     *  which do not pass boot drive correctly.
> +	     *  REM: the old STAGE1_BOOT_DRIVE_MASK equals STAGE1_BOOT_DRIVE_CHECK + 2.   */
> +         *((unsigned char *) (stage1_buffer + STAGE1_BOOT_DRIVE_CHECK + 2 ))
> +             = (dest_drive & BIOS_FLAG_FIXED_DISK);
> +	 }
> +       else
> +	    /* The boot sector is older than version 0.94.
> +	     * Changing 'goto fail' to a "nop" could even make 0.92 and 0.93
> +	     * acceptable for the purpose of rescuing their installations.     */
> +	 goto fail;
> +    }
> +
>    /* Read the first sector of Stage 2.  */
>    disk_read_hook = disk_read_savesect_func;
>    if (grub_read (stage2_first_buffer, SECTOR_SIZE) != SECTOR_SIZE)

The code is badly indented.

This code also looks great for upstream subimition. I would like if
you could send it to bug-grub at gnu.org and talk to them. If you can get
it there would be easier to us just grub a CVS snapshot for packaging
proposes.

After fixing the issues that I pointed out would be good to send it
upstream and check why _they_ broke the compatibility. Should exist a
reason for it.

-- 
        O T A V I O    S A L V A D O R
---------------------------------------------
 E-mail: otavio at debian.org      UIN: 5906116
 GNU/Linux User: 239058     GPG ID: 49A5F855
 Home Page: http://www.freedom.ind.br/otavio
---------------------------------------------
"Microsoft gives you Windows ... Linux gives
 you the whole house."