[RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Pavel Roskin]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

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

[-- Attachment #2: 01-patch-later.patch --]
[-- Type: text/x-patch, Size: 2464 bytes --]

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");

[-- Attachment #3: 01-write-early.patch --]
[-- Type: text/x-patch, Size: 1482 bytes --]

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.  */

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Bean]

On Sat, Jun 28, 2008 at 5:24 AM, Pavel Roskin <proski@gnu.org> wrote:
> 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.

Hi,

Here are some thoughts:

1, We can store the original content before making change.

2, Or when we modify data, we change both of the buffer, so that the
comparison would not fail.

-- 
Bean

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Bean]

On Sat, Jun 28, 2008 at 5:24 AM, Pavel Roskin <proski@gnu.org> wrote:
> I tend to prefer the later ("write early") approach, but I'd like to see
> some feedback before I commit it.

Hi,

The problem of write early is that it changes the target image before
verification is made, so if verification fails, the change remains.
This is generally not a good practice, we should not touch core.img if
it can't be embedded successfully.

-- 
Bean

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> 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.

Why not make a structure that holds every modification information that 
needs to be done ("configuration block"). And then you provide core.img 
and variable filled with configuration information to patch function 
that modifies core.img properly. This way you only need to write this once.

With this you can either modify compare or patch two copies of the 
images with same configuration information.

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Robert Millan]

On Fri, Jun 27, 2008 at 05:24:59PM -0400, Pavel Roskin wrote:
> 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.

Do we really want to support this?  I don't think there's any reliable way
to tell where filesystem metadata starts.  So unless we're talking about a
dedicated partition, this sounds like a no-go.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Fri, Jun 27, 2008 at 05:24:59PM -0400, Pavel Roskin wrote:
>> 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.
> 
> Do we really want to support this?  I don't think there's any reliable way
> to tell where filesystem metadata starts.  So unless we're talking about a
> dedicated partition, this sounds like a no-go.

This also relates to problem with installing to MBR and there is no room 
for embedding core.img there. I have been using USB stick where there is 
not enough room for core.img what I want to store there. So only option 
is to leave core.img to filesystem and install MBR.

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Robert Millan]

On Sun, Jun 29, 2008 at 04:23:24PM +0300, Vesa Jääskeläinen wrote:
> 
> This also relates to problem with installing to MBR and there is no room 
> for embedding core.img there. I have been using USB stick where there is 
> not enough room for core.img what I want to store there. So only option 
> is to leave core.img to filesystem and install MBR.

But then you can add an empty partition (type 0x00) starting at sector 63,
and grub-setup will overwrite it.

Or you could leave that region unpartitioned.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Sun, Jun 29, 2008 at 04:23:24PM +0300, Vesa Jääskeläinen wrote:
>> This also relates to problem with installing to MBR and there is no room 
>> for embedding core.img there. I have been using USB stick where there is 
>> not enough room for core.img what I want to store there. So only option 
>> is to leave core.img to filesystem and install MBR.
> 
> But then you can add an empty partition (type 0x00) starting at sector 63,
> and grub-setup will overwrite it.
> 
> Or you could leave that region unpartitioned.

At least on my disks there is 32 KiB free space. But not ~50 KiB what my 
core.img requires.

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Robert Millan]

On Sun, Jun 29, 2008 at 08:44:28PM +0300, Vesa Jääskeläinen wrote:
> Robert Millan wrote:
> >On Sun, Jun 29, 2008 at 04:23:24PM +0300, Vesa Jääskeläinen wrote:
> >>This also relates to problem with installing to MBR and there is no room 
> >>for embedding core.img there. I have been using USB stick where there is 
> >>not enough room for core.img what I want to store there. So only option 
> >>is to leave core.img to filesystem and install MBR.
> >
> >But then you can add an empty partition (type 0x00) starting at sector 63,
> >and grub-setup will overwrite it.
> >
> >Or you could leave that region unpartitioned.
> 
> At least on my disks there is 32 KiB free space.

Typically they're 32 kiB - 512 B (because most tools reserve that space
for some legacy restriction that no longer exists ;-)).

> But not ~50 KiB what my 
> core.img requires.

What do you put in your core?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [RFC PATCH] Sector 2 of non-embedded core.img: write early or patch later?

[Pavel Roskin]

On Sat, 2008-06-28 at 12:58 +0800, Bean wrote:
> On Sat, Jun 28, 2008 at 5:24 AM, Pavel Roskin <proski@gnu.org> wrote:
> > I tend to prefer the later ("write early") approach, but I'd like to see
> > some feedback before I commit it.
> 
> Hi,
> 
> The problem of write early is that it changes the target image before
> verification is made, so if verification fails, the change remains.
> This is generally not a good practice, we should not touch core.img if
> it can't be embedded successfully.

OK, that's a good argument.  Actually, if grub-install is used, core.img
would have been rewritten already.  Anyway, core.img is a file we want
to touch as little as possible, considering that it can already be used.

I'm applying the "patch-later" patch with some simplifications and
warning fixes.

-- 
Regards,
Pavel Roskin