I know this already got merged, but I still have some comments.

On Mon, Aug 12, 2013 at 10:19:30PM -0000, Tyler Baker wrote:
> Tyler Baker has proposed merging lp:~tyler-baker/lava-dispatcher/bootloader-next into lp:lava-dispatcher.
> 
> Requested reviews:
>   Linaro Validation Team (linaro-validation)
> Related bugs:
>   Bug #1211406 in LAVA Dispatcher: "Dispatcher not reading boot_cmds from image"
>   https://bugs.launchpad.net/lava-dispatcher/+bug/1211406
> 
> For more details, see:
> https://code.launchpad.net/~tyler-baker/lava-dispatcher/bootloader-next/+merge/179825
> 
> Introducing the BootloaderTarget class an extension of MasterImageTarget. This will allow all exisiting master image devices to function as they do today, however adds the ability to tftp boot binaries directly from the bootloader.
> 
> There is quite a few changes here..
> 
> * Add deploy_linaro_kernel
>   - New schema for dealing with binaries that are not in an image, or hwpack.
>   - Only required property is "kernel"
>   - Allows us to seperate the deployment/boot logic for raw kernel booting.
> 
> * BootloaderTarget Class
>   - Documentation MP is here https://code.launchpad.net/~tyler-baker/lava-project/bootloader/+merge/179796
>   - This class inherits from MasterImageTarget and implements deploy_linaro_kernel.
>   - Sets lava_server_ip, lava_kernel, lava_ramdisk, and lava_dtb as bootloader enviroment variables.
>   - It is up to the user to utilize these in their boot_cmds.
> 
> * Added a wrapper for wait_for_prompt as _wait_for_prompt in target.py
>   - This is a very common function, encouraging reuse without an import.
> 
> * _enter_bootloader added to target.py
>   - Refactored all the device types to use this single function.
> 
> * _customize_bootloader
>   - Fixed the pep8 issue with self.proc.
>   - Added _wait_for_prompt logic.
> 
> * Job defined boot_cmds
>   - Tested and is working on the following:
>      - Master Image Targets
>      - Bootloader Targets
>      - ARMv7 Fastmodels
> 
> * Refactored _boot_linaro_image in master.py
>   - The boot_cmds logic was a mess and was broken.
>      - https://bugs.launchpad.net/lava-dispatcher/+bug/1211406
>   - Confirmed this as a fix for 1211406
> 
> * Added client_type = bootloader to all Master Image devices
>   - This has been tested, and all devices function as they do today.
> 
> * Testing
>     - All test jobs passed.
>         - https://code.launchpad.net/~tyler-baker/+junk/bootloader-job-def-tests
> -- 
> https://code.launchpad.net/~tyler-baker/lava-dispatcher/bootloader-next/+merge/179825
> Your team Linaro Validation Team is requested to review the proposed merge of lp:~tyler-baker/lava-dispatcher/bootloader-next into lp:lava-dispatcher.

> === added file 'lava_dispatcher/device/bootloader.py'
> --- lava_dispatcher/device/bootloader.py	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/device/bootloader.py	2013-08-12 22:19:12 +0000
> @@ -0,0 +1,130 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Tyler Baker <email address hidden>
> +#
> +# This file is part of LAVA Dispatcher.
> +#
> +# LAVA Dispatcher is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# LAVA Dispatcher is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.    See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along
> +# with this program; if not, see <http://www.gnu.org/licenses>.
> +
> +import logging
> +import contextlib
> +import time
> +import os
> +import pexpect
> +
> +from lava_dispatcher.device.master import (
> +    MasterImageTarget
> +)
> +from lava_dispatcher.utils import (
> +    string_to_list
> +)
> +from lava_dispatcher.errors import (
> +    CriticalError
> +)
> +from lava_dispatcher.downloader import (
> +    download_image
> +)
> +
> +class BootloaderTarget(MasterImageTarget):
> +
> +    def __init__(self, context, config):
> +        super(BootloaderTarget, self).__init__(context, config)
> +        self._booted = False
> +        self._boot_cmds = None
> +        self._lava_cmds = None
> +        self._uboot_boot = False
> +        # This is the offset into the path, used to reference bootfiles
> +        self._offset = self.scratch_dir.index('images')

This does not look safe to me, we cannot assume that the configuration item
used by scratch_dir contains 'image' in its path. I think you need to store
dirname(self.scratch_dir) and then use that as a prefix to remove from the
filenames you need to make relative to some root.

> +
> +    def power_off(self, proc):
> +        if self._uboot_boot:
> +            if self.config.power_off_cmd:
> +                self.context.run_command(self.config.power_off_cmd)
> +        else:
> +            super(BootloaderTarget, self).power_off(proc)
> +
> +    def deploy_linaro_kernel(self, kernel, ramdisk, dtb, rootfs, bootloader):
> +         if bootloader == "u_boot":
> +             # We assume we will be controlling u-boot
> +             if kernel is not None:
> +                 # We have been passed kernel image, setup TFTP boot
> +                 self._uboot_boot = True
> +                 # Set the TFTP server IP (Dispatcher)
> +                 self._lava_cmds = "lava_server_ip=" + self.context.config.lava_server_ip + ","
> +                 kernel = download_image(kernel, self.context, self.scratch_dir, decompress=False)
> +                 # Set the TFTP bootfile path for the kernel
> +                 self._lava_cmds += "lava_kernel=" + kernel[self._offset::] + ","
> +                 if ramdisk is not None:
> +                     # We have been passed a ramdisk
> +                     ramdisk = download_image(ramdisk, self.context, self.scratch_dir, decompress=False)
> +                     # Set the TFTP bootfile path for the ramdisk
> +                     self._lava_cmds += "lava_ramdisk=" + ramdisk[self._offset::] + ","
> +                 if dtb is not None:
> +                     # We have been passed a device tree blob
> +                     dtb = download_image(dtb, self.context, self.scratch_dir, decompress=False)
> +                     # Set the bootfile path for the ramdisk
> +                     self._lava_cmds += "lava_dtb=" + dtb[self._offset::] + ","
> +                 if rootfs is not None:
> +                     # We have been passed a rootfs
> +                     rootfs = download_image(rootfs, self.context, self.scratch_dir, decompress=True)
> +                     self._lava_cmds += "lava_rootfs=" + dtb[self._offset::] + ","
> +                 else:
> +                     # TODO: Faking the deployment data - Ubuntu
> +                     self.deployment_data = self.target_map['ubuntu']
> +             else:
> +                 # This *should* never happen
> +                 raise CriticalError("No kernel images to boot")
> +         else:
> +             # Define other "types" of bootloaders here. UEFI? Grub?
> +             raise CriticalError("U-Boot is the only supported bootloader at this time")
> +
> +    def deploy_linaro(self, hwpack, rfs, bootloader):
> +        self._uboot_boot = False
> +        super(BootloaderTarget, self).deploy_linaro(hwpack, rfs, bootloader)
> +
> +    def deploy_linaro_prebuilt(self, image):
> +        self._uboot_boot = False
> +        super(BootloaderTarget, self).deploy_linaro_prebuilt(image)
> +
> +    def _inject_boot_cmds(self):
> +        if isinstance(self.config.boot_cmds, basestring):
> +            if self.config.boot_cmds_tftp is None:
> +                raise CriticalError("No TFTP boot commands defined")
> +            else:
> +                self._boot_cmds = self._lava_cmds + self.config.boot_cmds_tftp
> +                self._boot_cmds = string_to_list(self._boot_cmds.encode('ascii'))
> +        else:
> +            self._boot_cmds = string_to_list(self._lava_cmds.encode('ascii')) + self.config.boot_cmds
> +
> +    def _run_boot(self):
> +        self._enter_bootloader(self.proc)
> +        self._inject_boot_cmds()
> +        self._customize_bootloader(self.proc, self._boot_cmds)
> +        self._wait_for_prompt(self.proc, ['\(initramfs\)', self.config.master_str],
> +                        self.config.boot_linaro_timeout)
> +
> +    def _boot_linaro_image(self):
> +        if self._uboot_boot:
> +            if self.config.hard_reset_command:
> +                self._hard_reboot()
> +            else:
> +                raise CriticalError("No hard reset command defined")               
> +            self._run_boot()
> +            self.proc.sendline('export PS1="%s"' % self.deployment_data['TESTER_PS1'])
> +            self._booted = True
> +        else:
> +            super(BootloaderTarget, self)._boot_linaro_image()
> +
> +target_class = BootloaderTarget
> 
> === modified file 'lava_dispatcher/device/capri.py'
> --- lava_dispatcher/device/capri.py	2013-07-16 16:04:57 +0000
> +++ lava_dispatcher/device/capri.py	2013-08-12 22:19:12 +0000
> @@ -44,11 +44,11 @@
>              return
>          try:
>              self._soft_reboot()
> -            self._enter_bootloader()
> +            self._enter_bootloader(self.proc)
>          except:
>              logging.exception("_enter_bootloader failed")
>              self._hard_reboot()
> -            self._enter_bootloader()
> +            self._enter_bootloader(self.proc)
>          self.proc.sendline("fastboot")
>  
>      def deploy_android(self, boot, system, userdata):
> 
> === modified file 'lava_dispatcher/device/fastmodel.py'
> --- lava_dispatcher/device/fastmodel.py	2013-07-24 14:04:25 +0000
> +++ lava_dispatcher/device/fastmodel.py	2013-08-12 22:19:12 +0000
> @@ -47,6 +47,7 @@
>      extract_targz,
>      DrainConsoleOutput,
>      finalize_process,
> +    string_to_list,
>  )
>  
>  
> @@ -238,11 +239,6 @@
>          if self._uefi:
>              os.chown(self._uefi, st.st_uid, st.st_gid)
>  
> -    def _enter_bootloader(self):
> -        if self.proc.expect(self.config.interrupt_boot_prompt) != 0:
> -            raise Exception("Failed to enter bootloader")
> -        self.proc.sendline(self.config.interrupt_boot_command)
> -
>      def power_off(self, proc):
>          super(FastModelTarget, self).power_off(proc)
>          finalize_process(self._sim_proc)
> @@ -304,8 +300,12 @@
>              self.proc.logfile_read)
>  
>          if self._uefi:
> -            self._enter_bootloader()
> -            self._customize_bootloader()
> +            self._enter_bootloader(self.proc)
> +            if isinstance(self.config.boot_cmds, basestring):
> +                boot_cmds = string_to_list(self.config.boot_cmds.encode('ascii'))
> +            else:
> +                boot_cmds = self.config.boot_cmds
> +            self._customize_bootloader(self.proc, boot_cmds)
>  
>          return self.proc
>  
> 
> === modified file 'lava_dispatcher/device/master.py'
> --- lava_dispatcher/device/master.py	2013-07-17 09:16:46 +0000
> +++ lava_dispatcher/device/master.py	2013-08-12 22:19:12 +0000
> @@ -99,12 +99,14 @@
>          return self.device_version
>  
>      def power_on(self):
> +        if self.config.power_on_cmd:
> +            self.context.run_command(self.config.power_on_cmd)
>          self._boot_linaro_image()
>          return self.proc
>  
>      def power_off(self, proc):
> -        # we always leave master image devices powered on
> -        pass
> +        if self.config.power_off_cmd:
> +            self.context.run_command(self.config.power_off_cmd)
>  
>      def deploy_linaro(self, hwpack, rfs, bootloader):
>          self.boot_master_image()
> @@ -482,47 +484,51 @@
>              self.proc.sendline("hardreset")
>              self.proc.empty_buffer()
>  
> -    def _enter_bootloader(self):
> -        if self.proc.expect(self.config.interrupt_boot_prompt) != 0:
> -            raise Exception("Failed to enter bootloader")
> -        self.proc.sendline(self.config.interrupt_boot_command)
> -
>      def _boot_linaro_image(self):
>          boot_cmds = self.deployment_data['boot_cmds']
> -        boot_cmds_override = False
> -
>          options = boot_options.as_dict(self, defaults={'boot_cmds': boot_cmds})
> -        if 'boot_cmds' in options:
> +
> +        # Interactive boot_cmds from the job file are a list.
> +        # We check for them first, if they are present, we use
> +        # them and ignore the other cases.
> +        if not isinstance(self.config.boot_cmds, basestring):
> +            logging.info('Overriding boot_cmds from job file')
>              boot_cmds_override = True
> +            boot_cmds = self.config.boot_cmds
> +        # If there were no interactive boot_cmds, next we check
> +        # for boot_option overrides. If one exists, we use them
> +        # and ignore all other cases.
> +        elif options['boot_cmds'].value != 'boot_cmds':
> +            logging.info('Overriding boot_cmds from boot_options')
>              boot_cmds = options['boot_cmds'].value

when an if statement has an else clause I would avoid negative condition, so instead of

if not X:
  A
else:
  B

I suggest using

if X:
  B
else:
  A