Merge lp:~mwaddel/linaro-image-tools/null-string-vexpress into lp:linaro-image-tools/11.11

[Matt Waddel]

The call to make_boot_files() fails when boot_script_path isn't initialized. This happens in the Versatile Express because the boot script isn't used on this platform.

[Loïc Minier (lool)]

On Sat, Apr 02, 2011, Matt Waddel wrote:
> + if cls.boot_script:
> + boot_script_path = os.path.join(boot_disk, cls.boot_script)
> + else:
> + boot_script_path = ''

 Can we make it None instead? '' + 'boot.scr' might actually work and
 result in either '/boot.scr' on the host or 'boot.scr' in the current
 working directory on the host, so I prefer we use None just in case

 Perhaps something like:
    boot_script_path = None
    if cls.boot_script:
        boot_script_path = os.path.join(boot_disk, cls.boot_script)

--
Loïc Minier

[Guilherme Salgado (salgado)]

Hi Matt,

Thanks for this fix; I have just one suggestion below.

On Sat, 2011-04-02 at 05:21 +0000, Matt Waddel wrote:
[...]
> === modified file 'linaro_image_tools/media_create/boards.py'
> --- linaro_image_tools/media_create/boards.py 2011-04-01 14:18:41 +0000
> +++ linaro_image_tools/media_create/boards.py 2011-04-02 05:21:29 +0000
> @@ -287,7 +287,10 @@
> cmd_runner.run(
> ['cp', '-v', uboot_bin, boot_disk], as_root=True).wait()
>
> - boot_script_path = os.path.join(boot_disk, cls.boot_script)
> + if cls.boot_script:
> + boot_script_path = os.path.join(boot_disk, cls.boot_script)
> + else:
> + boot_script_path = ''

I think it'd make more sense to just move the line you removed to the
_make_boot_files() methods that use it. For instance, in OmapConfig
we'd have:

   boot_script_path = os.path.join(boot_dir, cls.boot_script)
   [...]
   make_boot_script(boot_env, boot_script_path)
   [...]

That means we'd duplicate the boot_script_path initialization in a few
different methods, but it's just one line and I think that's cleaner
than having it initialized in populate_boot() and passed all the way
down to _make_boot_files()

[lp:~mwaddel/linaro-image-tools/null-string-vexpress]

310. By Matt Waddel on 2011-04-04

Removes potentially uninitialized variable, boot_script_path, from calls to make_boot_files()

[Matt Waddel (mwaddel)]

Hi Salgado,

I have made the changes you suggested. The changes are a bit more extensive than the original fix, but I tested in both a beagle and a vexpress deploy and everything seems to work.

Beagle deploy command used:
./linaro-image-tools/linaro-media-create --mmc /dev/sdb --dev beagle --console ttyAMA0,38400n8 --binary ./images/linaro-natty-nano-tar-20110329-0.tar.gz --hwpack ./images/hwpack_linaro-omap3_20110330-0_armel_supported.tar.gz

Vexpress deploy command used:
./linaro-image-tools/linaro-media-create --mmc /dev/sdb --dev vexpress --console ttyAMA0,38400n8 --binary ./images/linaro-natty-nano-tar-20110329-0.tar.gz --hwpack ./images/hwpack_linaro-vexpress_20110329-0_armel_supported.tar.gz

I think it's ready for a merge review again.

[Guilherme Salgado (salgado)]

Thanks for doing the changes, Matt. I'm happy to merge this but I think some tests will fail because of the signature change in make_boot_files(). Did you run the test suite ('testr run') to check?

If I'm not mistaken, you'll just have to remove the 'boot_disk/boot_script' string from TestPopulateBoot.expected_args[live] in tests/test_media_create.py.

review: Approve

=== modified file 'linaro_image_tools/media_create/boards.py'
--- linaro_image_tools/media_create/boards.py	2011-04-01 14:18:41 +0000
+++ linaro_image_tools/media_create/boards.py	2011-04-04 16:38:27 +0000
@@ -250,16 +250,15 @@
 
     @classmethod
     def make_boot_files(cls, uboot_parts_dir, is_live, is_lowmem, consoles,
-                        chroot_dir, rootfs_uuid, boot_dir, boot_script_path,
-                        boot_device_or_file):
+                        chroot_dir, rootfs_uuid, boot_dir, boot_device_or_file):
         boot_env = cls._get_boot_env(is_live, is_lowmem, consoles, rootfs_uuid)
         cls._make_boot_files(
-            uboot_parts_dir, boot_env, chroot_dir, boot_dir, boot_script_path,
+            uboot_parts_dir, boot_env, chroot_dir, boot_dir, 
             boot_device_or_file)
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         """Make the necessary boot files for this board.
 
         This is usually board-specific so ought to be defined in every
@@ -287,11 +286,9 @@
             cmd_runner.run(
                 ['cp', '-v', uboot_bin, boot_disk], as_root=True).wait()
 
-        boot_script_path = os.path.join(boot_disk, cls.boot_script)
-
         cls.make_boot_files(
             uboot_parts_dir, is_live, is_lowmem, consoles, chroot_dir,
-            rootfs_uuid, boot_disk, boot_script_path, boot_device_or_file)
+            rootfs_uuid, boot_disk, boot_device_or_file)
 
         cmd_runner.run(['sync']).wait()
         try:
@@ -346,23 +343,23 @@
 
     @classmethod
     def make_boot_files(cls, uboot_parts_dir, is_live, is_lowmem, consoles,
-                        chroot_dir, rootfs_uuid, boot_dir, boot_script_path,
-                        boot_device_or_file):
+                        chroot_dir, rootfs_uuid, boot_dir, boot_device_or_file):
         # XXX: This is also part of our temporary hack to fix bug 697824; we
         # need to call set_appropriate_serial_tty() before doing anything that
         # may use cls.serial_tty.
         cls.set_appropriate_serial_tty(chroot_dir)
         super(OmapConfig, cls).make_boot_files(
             uboot_parts_dir, is_live, is_lowmem, consoles, chroot_dir,
-            rootfs_uuid, boot_dir, boot_script_path, boot_device_or_file)
+            rootfs_uuid, boot_dir, boot_device_or_file)
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         install_omap_boot_loader(chroot_dir, boot_dir)
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
         make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
+        boot_script_path = os.path.join(boot_dir, cls.boot_script)
         make_boot_script(boot_env, boot_script_path)
         make_boot_ini(boot_script_path, boot_dir)
 
@@ -414,10 +411,11 @@
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
         make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
+        boot_script_path = os.path.join(boot_dir, cls.boot_script)
         make_boot_script(boot_env, boot_script_path)
         make_boot_ini(boot_script_path, boot_dir)
 
@@ -440,10 +438,11 @@
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
         make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
+        boot_script_path = os.path.join(boot_dir, cls.boot_script)
         make_boot_script(boot_env, boot_script_path)
 
 
@@ -485,13 +484,14 @@
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         uboot_file = os.path.join(
             chroot_dir, 'usr', 'lib', 'u-boot', cls.uboot_flavor, 'u-boot.imx')
         install_mx5_boot_loader(uboot_file, boot_device_or_file)
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
         make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
+        boot_script_path = os.path.join(boot_dir, cls.boot_script)
         make_boot_script(boot_env, boot_script_path)
 
 
@@ -542,7 +542,7 @@
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
         make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
@@ -604,7 +604,7 @@
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
-                         boot_script_path, boot_device_or_file):
+                         boot_device_or_file):
         uboot_file = os.path.join(
             chroot_dir, 'usr', 'lib', 'u-boot', 'smdkv310', 'u-boot.v310')
         install_smdkv310_boot_loader(uboot_file, boot_device_or_file)
@@ -623,6 +623,7 @@
 
         # unused at the moment once FAT support enabled for the
         # Samsung u-boot this can be used bug 727978
+        #boot_script_path = os.path.join(boot_dir, cls.boot_script)
         #make_boot_script(boot_env, boot_script_path)