Hi Colin & Vladimir:

On 03/25/2011 06:31 PM, Colin Watson wrote:
> 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,
>
Thanks for the comments.  I agree fully with Colin's comments.  Here's an updated patch.

-- 
*Mario Limonciello*
Linux Engineer
*Dell* | OS Engineering