usb-creator

Code review comment for lp:~superm1/usb-creator/grub-support

grub-support
Merge into trunk

Revision history for this message

Colin Watson (cjwatson) wrote on 2010-09-03:

On Thu, Sep 02, 2010 at 11:49:55PM -0000, Mario Limonciello wrote:
> It makes an assumption that when grub is installed to CDs it will be
> installed in boot/grub/i386-pc. This assumption was based upon the
> way that EFI was implemented on AMD64 CDs for 10.10 that modules were
> in /boot/grub/x86_64-efi. If that's a bad assumption, this can be
> modified.

I think this assumption is OK.

> - num = deviceobj.Get(device, 'partition-number',
> - dbus_interface=PROPS_IFACE)
> - try:
> - popen(['/sbin/parted', parent, 'set', str(num), 'boot', 'on'])
> - except USBCreatorProcessException:
> - # Don't worry about not being able to re-read the partition table.
> - # TODO: As this will still be a problem for KVM users, this should
> - # be fixed by unmounting all the partitions before we get to this
> - # point, then remounting the target partition after.
> - pass

We need the parted call for both syslinux and GRUB. Some BIOSes refuse
to boot from a device that doesn't have an active partition.

> + if bootloader == 'grub':
> + core = tempfile.mktemp()
> + popen(['grub-mkimage', '-p', '/boot/grub/i386-pc', '-o', core,
> + 'biosdisk', 'part_msdos', 'fat', 'ntfs'])

This creates a GRUB image based on the version of grub-pc that you have
installed, on the assumption that it will continue to work with the old
configuration file on the CD image. This is not necessarily a sound
assumption. It also hardcodes module names; the set chosen isn't likely
to change all that much, but I feel it's worth mentioning.

If you use the installed version of grub-pc, then you must also copy
over modules from the installed system. The ABI between core.img and
modules changes frequently.

> + popen(['/usr/sbin/grub-setup', '-d', '/', '-b', '/usr/lib/grub/i386-pc/boot.img',
> + '-c', core, parent])

Hardcoded path to grub-setup. Also surely this should be '-d
/usr/lib/grub/i386-pc' rather than using both -d and -b? '-d /' makes
no sense.

In general, I have to say that I'm kind of uneasy with this approach; I
think it will break just as much as syslinux has been breaking, if not
more. I have an alternative suggestion. When we switch to GRUB, we
should arrange that the very same core.img on the CD contains the
necessary modules to boot on USB sticks as well. We can then simply dd
the existing core.img into the space after the MBR, and not have to
worry about these interface issues.

On Thu, Sep 02, 2010 at 11:49:55PM -0000, Mario Limonciello wrote:
> It makes an assumption that when grub is installed to CDs it will be
> installed in boot/grub/i386-pc.  This assumption was based upon the
> way that EFI was implemented on AMD64 CDs for 10.10 that modules were
> in /boot/grub/x86_64-efi.  If that's a bad assumption, this can be
> modified.

I think this assumption is OK.

> -        num = deviceobj.Get(device, 'partition-number',
> -                            dbus_interface=PROPS_IFACE)
> -        try:
> -            popen(['/sbin/parted', parent, 'set', str(num), 'boot', 'on'])
> -        except USBCreatorProcessException:
> -            # Don't worry about not being able to re-read the partition table.
> -            # TODO: As this will still be a problem for KVM users, this should
> -            # be fixed by unmounting all the partitions before we get to this
> -            # point, then remounting the target partition after.
> -            pass

We need the parted call for both syslinux and GRUB.  Some BIOSes refuse
to boot from a device that doesn't have an active partition.

> +        if bootloader == 'grub':
> +            core = tempfile.mktemp()
> +            popen(['grub-mkimage', '-p', '/boot/grub/i386-pc', '-o', core,
> +                    'biosdisk', 'part_msdos', 'fat', 'ntfs'])

This creates a GRUB image based on the version of grub-pc that you have
installed, on the assumption that it will continue to work with the old
configuration file on the CD image.  This is not necessarily a sound
assumption.  It also hardcodes module names; the set chosen isn't likely
to change all that much, but I feel it's worth mentioning.

If you use the installed version of grub-pc, then you must also copy
over modules from the installed system.  The ABI between core.img and
modules changes frequently.

> +            popen(['/usr/sbin/grub-setup', '-d', '/', '-b', '/usr/lib/grub/i386-pc/boot.img',
> +                   '-c', core, parent])

Hardcoded path to grub-setup.  Also surely this should be '-d
/usr/lib/grub/i386-pc' rather than using both -d and -b?  '-d /' makes
no sense.

In general, I have to say that I'm kind of uneasy with this approach; I
think it will break just as much as syslinux has been breaking, if not
more.  I have an alternative suggestion.  When we switch to GRUB, we
should arrange that the very same core.img on the CD contains the
necessary modules to boot on USB sticks as well.  We can then simply dd
the existing core.img into the space after the MBR, and not have to
worry about these interface issues.

« Back to merge proposal