On Mon, Jun 14, 2010 at 02:25:39PM +0100, Colin Watson wrote: > On Mon, Jun 14, 2010 at 08:07:35AM -0500, richardvo...@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 <cjwat...@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 [cjwat...@ubuntu.com] -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org