Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson <cjwatson@ubuntu.com>]

On Mon, Jun 14, 2010 at 02:25:39PM +0100, Colin Watson wrote:
> On Mon, Jun 14, 2010 at 08:07:35AM -0500, richardvoigt@gmail.com wrote:
> > Colin Watson wrote:
> > > I can think of an alternative.  We do still need grub_install_dos_part
> > > and grub_install_bsd_part for the multiboot trampoline, which is in
> > > assembly, so it's difficult to abandon them altogether.  However,
> > > there's no reason we need to use them in make_install_device.  How about
> > > we invent a way to encode most of the information in string form in
> > > grub_prefix, while leaving a placeholder for make_install_device to fill
> > > in the disk?  There are 64 bytes available for grub_prefix, which should
> > > be plenty.  For example, how about the following (with \0 standing for
> > > ASCII NUL):
> > >
> > >  (\0,msdos1,bsd1)/boot/grub
> > >
> > > It's then just a matter of spotting the "(\0," sequence and replacing
> > > the \0 with the drive name.  I think this can probably be done using
> > > less code than the first option above, and all told it feels a bit less
> > > hacky to me.
> > 
> > Instead of doing string replacement, why not just define a
> > disk-relative partition format?
> 
> Simple string replacement would be much easier to implement, and
> probably smaller.  Plus, we don't need disk-relative device naming
> elsewhere, and I think it would require putting platform-specific code
> (otherwise how do you know which disk to be relative to?) in places that
> are otherwise pretty platform-independent.
> 
> > Also, the '\0' seems unnecessary.  Is having "(," meaningful in some
> > way already?
> 
> Good point.  This would be sufficient.

How about the following patch, implementing this proposal?  I've tested this
with Debian GNU/kFreeBSD, and it's enough to make the boot loader work again
out of the box after grub-install.  The 'root' variable is still wrong, but
that isn't particularly urgent as UUIDs usually take care of that.

The kernel.img size increase is 84 bytes, yielding a core.img size
increase of 50 bytes in this configuration (22932 -> 22982 bytes).  Do I
need to work on making that smaller somehow?  It seems tolerable to me.

2010-06-14  Colin Watson  <cjwatson@ubuntu.com>

	Fix i386-pc prefix handling with nested partitions (Debian bug
	#585068).

	* kern/i386/pc/init.c (make_install_device): If the prefix starts
	with "(,", fill the boot drive in between those two characters, but
	expect that a full partition specification including partition map
	names will follow.
	* util/i386/pc/grub-setup.c (setup): Unless an explicit prefix was
	specified, write a prefix without the drive name but including a
	full partition specification.

=== modified file 'kern/i386/pc/init.c'
--- kern/i386/pc/init.c	2010-05-21 18:08:48 +0000
+++ kern/i386/pc/init.c	2010-06-14 14:44:13 +0000
@@ -83,6 +83,14 @@ make_install_device (void)
       grub_snprintf (ptr, sizeof (dev) - (ptr - dev), ")%s", grub_prefix);
       grub_strcpy (grub_prefix, dev);
     }
+  else if (grub_prefix[1] == ',')
+    {
+      /* We have a prefix, but still need to fill in the boot drive.  */
+      grub_snprintf (dev, sizeof (dev),
+		     "(%cd%u%s", (grub_boot_drive & 0x80) ? 'h' : 'f',
+		     grub_boot_drive & 0x7f, grub_prefix + 1);
+      grub_strcpy (grub_prefix, dev);
+    }
 
   return grub_prefix;
 }

=== modified file 'util/i386/pc/grub-setup.c'
--- util/i386/pc/grub-setup.c	2010-06-11 20:31:16 +0000
+++ util/i386/pc/grub-setup.c	2010-06-14 14:45:24 +0000
@@ -99,6 +99,7 @@ setup (const char *dir,
   struct grub_boot_blocklist *first_block, *block;
   grub_int32_t *install_dos_part, *install_bsd_part;
   grub_int32_t dos_part, bsd_part;
+  char *prefix;
   char *tmp_img;
   int i;
   grub_disk_addr_t first_sector;
@@ -230,6 +231,8 @@ setup (const char *dir,
 				       + GRUB_KERNEL_MACHINE_INSTALL_DOS_PART);
   install_bsd_part = (grub_int32_t *) (core_img + GRUB_DISK_SECTOR_SIZE
 				       + GRUB_KERNEL_MACHINE_INSTALL_BSD_PART);
+  prefix = (char *) (core_img + GRUB_DISK_SECTOR_SIZE +
+		     GRUB_KERNEL_MACHINE_PREFIX);
 
   /* Open the root device and the destination device.  */
   root_dev = grub_device_open (root);
@@ -289,21 +292,45 @@ setup (const char *dir,
      override the current setting.  */
   if (*install_dos_part != -2)
     {
+      grub_partition_t root_part = root_dev->disk->partition;
+
       /* Embed information about the installed location.  */
-      if (root_dev->disk->partition)
+      if (root_part)
 	{
-	  if (root_dev->disk->partition->parent)
+	  char *new_prefix;
+
+	  if (root_part->parent)
  	    {
-	      if (root_dev->disk->partition->parent->parent)
+	      if (root_part->parent->parent)
 		grub_util_error ("Installing on doubly nested partitions is "
 				 "not supported");
-	      dos_part = root_dev->disk->partition->parent->number;
-	      bsd_part = root_dev->disk->partition->number;
+	      dos_part = root_part->parent->number;
+	      bsd_part = root_part->number;
+
+	      if (prefix[0] != '(')
+		{
+		  new_prefix = xasprintf ("(,%s%d,%s%d)%s",
+					  root_part->parent->partmap->name,
+					  root_part->parent->number + 1,
+					  root_part->partmap->name,
+					  root_part->number + 1,
+					  prefix);
+		  strcpy (prefix, new_prefix);
+		}
  	    }
 	  else
  	    {
-	      dos_part = root_dev->disk->partition->number;
+	      dos_part = root_part->number;
  	      bsd_part = -1;
+
+	      if (prefix[0] != '(')
+		{
+		  new_prefix = xasprintf ("(,%s%d)%s",
+					  root_part->partmap->name,
+					  root_part->number + 1,
+					  prefix);
+		  strcpy (prefix, new_prefix);
+		}
  	    }
 	}
       else

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]