Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Mon, Jun 14, 2010 at 05:58:55PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> On 06/14/2010 05:02 PM, Colin Watson wrote:
> > 2010-06-14  Colin Watson  <address@hidden>
> >
> >     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.
> 
> That makes a regression for multiboot loading of image if it was moved
> from one partition to another.

Do you have any suggestions on how to deal with that?  I'm not familiar
with multiboot and need guidance.

> > === 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);
> > +    }
> 
> Or grub_prefix[1] == ')' to allow ()/boot/grub syntax for unpartitioned
> devices

Makes sense.  I've updated my local tree.

> > +     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");
> >   
> 
> This error must go away with that new syntax.

I agree, but how should kern/i386/pc/startup.S:multiboot_trampoline
cope?  I left this there because I didn't know a straightforward way to
deal with that.

> > -         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);
> > +           }
> >   
> 
> Please use grub_partition_get_name. It will greatly simplify this part.

Done, thanks.  (I had a memory leak too.)

Updated patch follows; same ChangeLog.

=== 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 16:21:53 +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] == ',' || 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 16:29:54 +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);
@@ -305,6 +308,18 @@ setup (const char *dir,
              dos_part = root_dev->disk->partition->number;
              bsd_part = -1;
            }
+
+         if (prefix[0] != '(')
+           {
+             char *root_part_name, *new_prefix;
+
+             root_part_name =
+               grub_partition_get_name (root_dev->disk->partition);
+             new_prefix = xasprintf ("(,%s)%s", root_part_name, prefix);
+             strcpy (prefix, new_prefix);
+             free (new_prefix);
+             free (root_part_name);
+           }
        }
       else
        dos_part = bsd_part = -1;

-- 
Colin Watson                                       address@hidden