Re: [PATCH] grub-setup Modify the conditionality of the copy of the partition table

[Colin Watson]

On Fri, Mar 25, 2011 at 11:55:32PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> On 25.03.2011 23:13, Mario Limonciello wrote:
> > +        * util/grub-setup.c: Conditionalize the partition map copy on 
> > floppy
> > +          support, not on whether the target contains partitions.
> > +
> > +          Otherwise, the BIOS on Dell Latitude E series laptops will 
> > freeze 
> > +          during POST if an invalid partition table is contained in the PBR
> > +          of the active partition when GRUB is installed to a partition.
> 
> It's wrong to assume that no floppies contain partition tables. Also
> --allow-floppy is usually used for USB sticks which sometimes appear as
> floppies on some BIOSes and they pretty much contain the partition
> table. Bottom line is: if you see a partition table: preserve it.

Reading from floppies requires the floppy_probe function in boot.S,
doesn't it, which collides with the partition table?  But it makes sense
to keep dest_partmap in the condition for safety anyway (and it saves a
call to grub_util_biosdisk_is_floppy), so we'd end up with:

    if (dest_partmap ||
        (!allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk)))
      memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
              tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
              GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC);

?

> But it's ok to preserve this contents if floppy support is disabled
> even if no partition table is discovered.

In the case Mario discovered, there was indeed no partition table (all
zeroes).  Writing the floppy-probing code into that area caused it to
fail.

> A small logic lecture:
> 
> Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy 
> (dest_dev->disk)
> Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy 
> (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy 
> (dest_dev->disk), not what you wrote.

Agreed.  The latter form is found elsewhere in grub-setup.

> Please don't move around unrelated parts.

The parts Mario moved around weren't unrelated.  Since the case at hand
is that of installing to a partition boot record, this if block will
run:

    if (! dest_partmap)
      {
        grub_util_warn (_("Attempting to install GRUB to a partitionless disk 
or to a partition.  This is a BAD idea."));
        goto unable_to_embed;
      }

In order to preserve the partition table (or lack thereof) in this case,
it was necessary to move the memcpy up above that test.

Cheers,

-- 
Colin Watson                                       address@hidden