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

[Mario Limonciello <mario_limonciello@dell.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2886 bytes --]

 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

[-- Attachment #1.1.2: Type: text/html, Size: 4743 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: always_copy_partition_table.patch --]
[-- Type: text/x-diff; name="always_copy_partition_table.patch", Size: 2266 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2011-03-23 19:29:17 +0000
+++ ChangeLog	2011-03-25 21:56:23 +0000
@@ -1,3 +1,11 @@
+2011-03-24  Mario Limonciello    <Mario_Limonciello@Dell.com>
+        * 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.
+
 2011-03-23  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* grub-core/term/gfxterm.c (calculate_normal_character_width): Return 8

=== modified file 'util/grub-setup.c'
--- util/grub-setup.c	2011-01-07 12:27:34 +0000
+++ util/grub-setup.c	2011-03-26 00:31:58 +0000
@@ -399,25 +399,26 @@
       }
 #endif
 
-    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;
-      }
-    if (multiple_partmaps || fs)
-      {
-	grub_util_warn (_("Attempting to install GRUB to a disk with multiple partition labels or both partition label and filesystem.  This is not supported yet."));
-	goto unable_to_embed;
-      }
-
     /* Copy the partition table.  */
-    if (dest_partmap)
+    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);
 
     free (tmp_img);
     
+    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;
+      }
+    if (multiple_partmaps || fs)
+      {
+	grub_util_warn (_("Attempting to install GRUB to a disk with multiple partition labels or both partition label and filesystem.  This is not supported yet."));
+	goto unable_to_embed;
+      }
+
     if (!dest_partmap->embed)
       {
 	grub_util_warn ("Partition style '%s' doesn't support embeding",


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]