From: Pavel Roskin <proski@gnu.org>
To: The development of GRUB 2 <grub-devel@gnu.org>
Content-Type: multipart/mixed; boundary="=-Nl8sivNpKRtm+Lu+AYVS"
Date: Fri, 27 Jun 2008 17:24:59 -0400
Message-Id: <1214601899.9513.58.camel@dv>
Mime-Version: 1.0
Subject: [RFC PATCH] Sector 2 of non-embedded core.img: write early or
	patch later?
Precedence: list
Reply-To: The development of GRUB 2 <grub-devel@gnu.org>


--=-Nl8sivNpKRtm+Lu+AYVS
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

Hello!

We have a serious problem with installing onto partitions (e.g.
grub-install /dev/sda1) and drives with geometry that doesn't leave
enough space for core.img to be embedded before the first partition.
While it's not a desirable configuration, it should work, but it
doesn't.

Either we should write sector 2 before the core.img is checked for
readability or we should delay patching sector 2 in memory until the
verification is done.  Otherwise, the verification will fail.

Delayed patching changes nothing in terms of I/O but needs a longer
patch and won't scale well if we want to put more information into
sector 2, as we'll need to cache more data.

Writing sector 2 early is less intrusive in terms of code, but may be
slower e.g. on floppies.  It's probably more reliable because we verify
that the modified sector 2 made it to the filesystem.

I tend to prefer the later ("write early") approach, but I'd like to see
some feedback before I commit it.

Both proposed patches are attached.

-- 
Regards,
Pavel Roskin

--=-Nl8sivNpKRtm+Lu+AYVS
Content-Disposition: attachment; filename=01-patch-later.patch
Content-Type: text/x-patch; name=01-patch-later.patch; charset=ANSI_X3.4-1968
Content-Transfer-Encoding: 7bit

diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 62c1bf1..3e9b4b5 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -101,6 +101,7 @@ setup (const char *dir,
   grub_uint16_t *boot_drive_check;
   struct boot_blocklist *first_block, *block;
   grub_int32_t *install_dos_part, *install_bsd_part;
+  grub_int32_t dos_part, bsd_part;
   char *tmp_img;
   int i;
   grub_disk_addr_t first_sector;
@@ -283,27 +284,24 @@ setup (const char *dir,
 	    {
 	      struct grub_pc_partition *pcdata =
 		root_dev->disk->partition->data;
-	      *install_dos_part
-		= grub_cpu_to_le32 (pcdata->dos_part);
-	      *install_bsd_part
-		= grub_cpu_to_le32 (pcdata->bsd_part);
+	      dos_part = grub_cpu_to_le32 (pcdata->dos_part);
+	      bsd_part = grub_cpu_to_le32 (pcdata->bsd_part);
 	    }
 	  else if (strcmp (root_dev->disk->partition->partmap->name,
 			   "gpt_partition_map") == 0)
 	    {
-	      *install_dos_part = grub_cpu_to_le32 (root_dev->disk->partition->index);
-	      *install_bsd_part = grub_cpu_to_le32 (-1);
+	      dos_part = grub_cpu_to_le32 (root_dev->disk->partition->index);
+	      bsd_part = grub_cpu_to_le32 (-1);
 	    }
 	  else
 	    grub_util_error ("No PC style partitions found");
 	}
       else
-	*install_dos_part = *install_bsd_part = grub_cpu_to_le32 (-1);
+	dos_part = bsd_part = grub_cpu_to_le32 (-1);
     }
   
   grub_util_info ("dos partition is %d, bsd partition is %d",
-		  grub_le_to_cpu32 (*install_dos_part),
-		  grub_le_to_cpu32 (*install_bsd_part));
+		  grub_le_to_cpu32 (dos_part), grub_le_to_cpu32 (bsd_part));
   
   /* If the destination device can have partitions and it is the MBR,
      try to embed the core image into after the MBR.  */
@@ -316,6 +314,9 @@ setup (const char *dir,
 	{
 	  grub_util_info ("will embed the core image at sector 0x%llx", embed_region.start);
 
+	  *install_dos_part = dos_part;
+	  *install_bsd_part = bsd_part;
+
 	  /* The first blocklist contains the whole sectors.  */
 	  first_block->start = grub_cpu_to_le64 (embed_region.start + 1);
 	  first_block->len = grub_cpu_to_le16 (core_sectors - 1);
@@ -485,6 +486,9 @@ setup (const char *dir,
      the boot device.  */
   *root_drive = 0xFF;
 
+  *install_dos_part = dos_part;
+  *install_bsd_part = bsd_part;
+
   /* Write the first two sectors of the core image onto the disk.  */
   grub_util_info ("opening the core image `%s'", core_path);
   fp = fopen (core_path, "r+b");

--=-Nl8sivNpKRtm+Lu+AYVS
Content-Disposition: attachment; filename=01-write-early.patch
Content-Type: text/x-patch; name=01-write-early.patch; charset=ANSI_X3.4-1968
Content-Transfer-Encoding: 7bit

diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 62c1bf1..d2c5a73 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -365,6 +365,15 @@ setup (const char *dir,
   /* The core image must be put on a filesystem unfortunately.  */
   grub_util_info ("will leave the core image on the filesystem");
   
+  /* Write the second sector of the core image onto the disk.  */
+  grub_util_info ("opening the core image `%s'", core_path);
+  fp = fopen (core_path, "r+b");
+  if (! fp)
+    grub_util_error ("Cannot open `%s'", core_path);
+  grub_util_write_image_at (core_img + GRUB_DISK_SECTOR_SIZE,
+			    GRUB_DISK_SECTOR_SIZE, GRUB_DISK_SECTOR_SIZE, fp);
+  fclose (fp);
+
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
   core_path_dev = grub_util_get_path (DEFAULT_DIRECTORY, core_file);
@@ -485,13 +494,13 @@ setup (const char *dir,
      the boot device.  */
   *root_drive = 0xFF;
 
-  /* Write the first two sectors of the core image onto the disk.  */
+  /* Write the first sector of the core image onto the disk.  */
   grub_util_info ("opening the core image `%s'", core_path);
   fp = fopen (core_path, "r+b");
   if (! fp)
     grub_util_error ("Cannot open `%s'", core_path);
 
-  grub_util_write_image (core_img, GRUB_DISK_SECTOR_SIZE * 2, fp);
+  grub_util_write_image (core_img, GRUB_DISK_SECTOR_SIZE, fp);
   fclose (fp);
 
   /* Write the boot image onto the disk.  */

--=-Nl8sivNpKRtm+Lu+AYVS--