Merge lp:~superm1/usb-creator/grub-support into lp:usb-creator

[Mario Limonciello]

This adds early support for installing using grub rather than syslinux (when applicable) to avoid getting into the same situation as had happened with 10.10 changing syntax of the syslinux.cfg.

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.

[Colin Watson (cjwatson)]

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.

[lp:~superm1/usb-creator/grub-support]

319. By Colin Watson on 2010-09-03

GTK frontend: don't grey out "Make Startup Disk" when the source is a
physical CD.

[Mario Limonciello (superm1)]

This should hopefully address all of the concerns that Colin raised (but Launchpad decided to eat).

[lp:~superm1/usb-creator/grub-support]

320. By Mario Limonciello on 2010-09-04

Grow support for installing GRUB to USB sticks if it's detected in the image
rather than isolinux.

[Mario Limonciello (superm1)]

I did a couple of tests with handcrafted boot.img and core.img files, and this appears to work properly for them using dd now.

[Mario Limonciello (superm1)]

After more testing i've merged this in today.

The boot.img was used as it existed on the system.
The handcrafted core.img was generated like this:

#grub-mkimage -c grub.cfg -o core.img biosdisk part_msdos fat search_fs_file iso9660

grub.cfg:
     search.file /boot/grub/i386-pc/grub.cfg root
     set prefix=($root)/boot/grub/i386-pc

=== modified file 'bin/usb-creator-helper'
--- bin/usb-creator-helper	2010-04-01 13:11:48 +0000
+++ bin/usb-creator-helper	2010-09-04 00:08:40 +0000
@@ -18,6 +18,7 @@
 import gobject
 import dbus.service
 import logging
+import os
 logging.basicConfig(level=logging.DEBUG)
 from dbus.mainloop.glib import DBusGMainLoop
 from usbcreator.misc import *
@@ -67,15 +68,23 @@
         self.polkit = None
 
     # TODO return boolean success
-    @dbus.service.method(USBCREATOR_IFACE, in_signature='s', out_signature='',
+    @dbus.service.method(USBCREATOR_IFACE, in_signature='ss', out_signature='',
                          sender_keyword='sender', connection_keyword='conn')
-    def InstallBootloader(self, device, sender=None, conn=None):
-        '''Installs syslinux to the partition boot code area and writes the
+    def InstallBootloader(self, device, grub_location, sender=None, conn=None):
+        '''Install a bootloader to the boot code area, either grub or syslinux.
+
+           The function takes a partition device file of the form /dev/sda1
+           and an option grub_location argument for where grub is located
+
+           For GRUB, it's expected that GRUB already exists, all that is
+           installed is the bootsector code from grub-setup.
+
+           For syslinux:
+           Installs syslinux to the partition boot code area and writes the
            syslinux boot code to the disk code area.  The latter is done to
            handle cases where a bootloader is accidentally installed to the
-           MBR, and to handle some buggy BIOSes.
+           MBR, and to handle some buggy BIOSes.'''
 
-           The function takes a partition device file of the form /dev/sda1.'''
 
         self.check_polkit(sender, conn, 'com.ubuntu.usbcreator.bootloader')
         check_system_internal(device)
@@ -87,17 +96,26 @@
         udisks = dbus.Interface(udisks, DISKS_IFACE)
         device = udisks.FindDeviceByDeviceFile(device)
         deviceobj = bus.get_object(DISKS_IFACE, device)
-        
-        popen(['syslinux', '-f', device_file])
+
         # Find the parent of the partition.
         parent = deviceobj.Get(device, 'partition-slave',
                                dbus_interface=PROPS_IFACE)
         parentobj = bus.get_object(DISKS_IFACE, parent)
         parent = parentobj.Get(parent, 'device-file',
                                dbus_interface=PROPS_IFACE)
-        # Write the syslinux MBR.
-        popen(['dd', 'if=/usr/lib/syslinux/mbr.bin', 'of=%s' % parent,
-               'bs=446', 'count=1', 'conv=sync'])
+        
+        if grub_location:
+            #Expect that boot.img and core.img both pre-built in grub_location
+            popen(['dd', 'if=%s' % os.path.join(grub_location, 'boot.img'), 'of=%s' % parent,
+                   'bs=446', 'count=1', 'conv=sync'])
+            popen(['dd', 'if=%s' % os.path.join(grub_location, 'core.img'), 'of=%s' % parent,
+                   'bs=512', 'count=62', 'seek=1', 'conv=sync']) 
+        else:
+            popen(['syslinux', '-f', device_file])
+            # Write the syslinux MBR.
+            popen(['dd', 'if=/usr/lib/syslinux/mbr.bin', 'of=%s' % parent,
+                   'bs=446', 'count=1', 'conv=sync'])
+
         num = deviceobj.Get(device, 'partition-number',
                             dbus_interface=PROPS_IFACE)
         try:

=== modified file 'debian/changelog'
--- debian/changelog	2010-09-03 16:10:16 +0000
+++ debian/changelog	2010-09-04 00:08:40 +0000
@@ -3,6 +3,8 @@
   [ Mario Limonciello ]
   * Mangle whether the 'ui' keyword is in syslinux.cfg based on the OS version.
     (LP: #608382)
+  * Grow support for installing GRUB to USB sticks if it's detected in the image
+    rather than isolinux.
 
   [ Colin Watson ]
   * GTK frontend: don't grey out "Make Startup Disk" when the source is a

=== modified file 'usbcreator/install.py'
--- usbcreator/install.py	2010-08-31 18:33:45 +0000
+++ usbcreator/install.py	2010-09-04 00:08:40 +0000
@@ -185,7 +185,7 @@
         if os.path.exists(ldlinux):
             os.remove(ldlinux)
 
-    def install_bootloader(self):
+    def install_bootloader(self, grub_location=None):
         logging.debug('install_bootloader')
         self.progress_pulse()
         self.progress_message(_('Installing the bootloader...'))
@@ -207,7 +207,7 @@
                 bus = dbus.SystemBus()
                 obj = bus.get_object('com.ubuntu.USBCreator',
                                      '/com/ubuntu/USBCreator')
-                obj.InstallBootloader(self.device,
+                obj.InstallBootloader(self.device, grub_location,
                                       dbus_interface='com.ubuntu.USBCreator',
                                       timeout=MAX_DBUS_TIMEOUT)
             except dbus.DBusException:
@@ -419,7 +419,6 @@
         self.remove_extras()
         
         self.initialize_progress_thread()
-        self.install_bootloader()
 
         # Copy.
         
@@ -476,8 +475,14 @@
                 # and line[1] (HH:MM:SS).
                 logging.debug('mkdir %s' % os.path.join(self.target, line[2]))
                 os.mkdir(os.path.join(self.target, line[2]))
-        
-        self.mangle_syslinux()
+
+        grub = os.path.join(self.target, 'boot', 'grub', 'i386-pc')
+        if os.path.isdir(grub):
+            self.install_bootloader(grub)
+        else:
+            self.install_bootloader()
+            self.mangle_syslinux()
+
         self.create_persistence()
         self.sync()
 
@@ -507,8 +512,7 @@
         self.check()
         
         self.initialize_progress_thread()
-        self.install_bootloader()
-
+        
         self.progress_message(_('Copying files...'))
         for dirpath, dirnames, filenames in os.walk(self.source):
             sp = dirpath[len(self.source.rstrip(os.path.sep))+1:]
@@ -542,8 +546,14 @@
                     if os.path.exists(targetpath):
                         os.unlink(targetpath)
                     self.copy_file(sourcepath, targetpath)
-        
-        self.mangle_syslinux()
+
+        grub = os.path.join(self.target, 'boot', 'grub', 'i386-pc')
+        if os.path.isdir(grub):
+            self.install_bootloader(grub)
+        else:
+            self.install_bootloader()
+            self.mangle_syslinux()
+
         self.create_persistence()
         self.sync()