Hey Guys,

I turned this into a proper merge proposal on LP so we have a way to 
discuss the changes. My comments are below, basically - I'm pretty sure 
we can dramatically simplify this to be a very small change (my guess 
<15 lines of code).

On 11/30/2012 09:54 AM, Andy Doan wrote:
> === modified file 'lava_dispatcher/actions/boot_control.py'
> --- lava_dispatcher/actions/boot_control.py	2012-11-20 21:22:17 +0000
> +++ lava_dispatcher/actions/boot_control.py	2012-11-30 15:53:29 +0000
> @@ -70,6 +70,28 @@
>           finally:
>               self.context.test_data.add_result("boot_image", status)
>
> +### UEFI
> +
> +class cmd_boot_linaro_uefi_image(BaseAction):
> +    """ Call client code to boot to the master image
> +    """
> +
> +    parameters_schema = _boot_schema
> +
> +    def run(self,options=[]):
> +        client = self.client
> +        client.target_device.boot_options = options
> +        status = 'pass'
> +        try:
> +            client.boot_linaro_uefi_image()
> +        except:
> +            logging.exception("boot_linaro_image failed")
> +            status = 'fail'
> +            raise CriticalError("Failed to boot uefi test image.")
> +        finally:
> +            self.context.test_data.add_result("boot_uefi_image",status)
> +
> +### UEFI

I don't think this is needed. See my comments for the deploy logic in 
master.py below.

> === modified file 'lava_dispatcher/actions/deploy.py'
> --- lava_dispatcher/actions/deploy.py	2012-11-20 13:34:19 +0000
> +++ lava_dispatcher/actions/deploy.py	2012-11-30 15:53:29 +0000
> @@ -91,6 +91,20 @@
>       def run(self, boot, system, data, rootfstype='ext4'):
>           self.client.deploy_linaro_android(boot, system, data, rootfstype)
>
> +## UEFI ##
> +class cmd_deploy_linaro_uefi_image(BaseAction):
> +    parameters_schema = {
> +        'type': 'object',
> +        'properties' : {
> +            'hwpack': {'type':'string','optional': True},
> +            'rootfs' : {'type':'string','optional': True},
> +            },
> +        'additionalProperties': False,
> +        }
> +
> +    def run(self, hwpack, rootfs):
> +        self.client.deploy_linaro_uefi(hwpack,rootfs)
> +## UEFI ##

I don't think this is needed. See my comments for the deploy logic in 
master.py below.

> === modified file 'lava_dispatcher/client/base.py'
> --- lava_dispatcher/client/base.py	2012-11-29 08:54:47 +0000
> +++ lava_dispatcher/client/base.py	2012-11-30 15:53:29 +0000
> @@ -402,6 +402,20 @@
>           self.setup_proxy(TESTER_PS1_PATTERN)
>           logging.info("System is in test image now")
>
> +### UEFI
> +    def boot_linaro_uefi_image(self):
> +        """
> +        Reboot the system to the uefi test image
> +        """
> +        logging.info("Boot the uefi test image")
> +
> +        self._boot_linaro_uefi_image()
> +
> +        wait_for_prompt(self.proc,'/bin/sh', timeout=300)
> +        logging.info("System is in test image now")
> +
> +### UEFI
> +

I don't think this is needed. See my comments for the deploy logic in 
master.py below.

> === modified file 'lava_dispatcher/client/targetdevice.py'
> --- lava_dispatcher/client/targetdevice.py	2012-11-21 22:07:45 +0000
> +++ lava_dispatcher/client/targetdevice.py	2012-11-30 15:53:29 +0000
> @@ -50,6 +50,10 @@
>       def deploy_linaro_android(self, boot, system, data, rootfstype='ext4'):
>           self.target_device.deploy_android(boot, system, data)
>
> +## UEFI ##
> +    def deploy_linaro_uefi(self,hwpack,rootfs):
> +        self.target_device.deploy_uefi(hwpack,rootfs)
> +## UEFI ##

I don't think this is needed. See my comments for the deploy logic in 
master.py below.


>       def deploy_linaro(self, hwpack=None, rootfs=None, image=None,
>                         rootfstype='ext3', bootloader='u_boot'):
>           if image is None:
> @@ -68,6 +72,11 @@
>       def _boot_linaro_image(self):
>           self.proc = self.target_device.power_on()
>
> +### UEFI
> +    def _boot_linaro_uefi_image(self):
> +        self.proc = self.target_device.uefi_power_on()
> +
> +### UEFI

I don't think this is needed. See my comments for the deploy logic in 
master.py below.

> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py	2012-11-30 01:54:29 +0000
> +++ lava_dispatcher/config.py	2012-11-30 15:53:29 +0000
> @@ -73,6 +73,7 @@
>       simulator_version_command = schema.StringOption()
>       simulator_command = schema.StringOption()
>       simulator_axf_files = schema.ListOption()
> +    boot_cmds_uefi = schema.StringOption(fatal=True)  ### UEFI

This seems bad, why not just do
+    boot_cmds_uefi = schema.StringOption(default='')
Then you don't have to make a change to device-defaults.conf below.
Also - drop the "#UEFI" comments - we have revision control for a reason.

> === modified file 'lava_dispatcher/default-config/lava-dispatcher/device-defaults.conf'
> --- lava_dispatcher/default-config/lava-dispatcher/device-defaults.conf	2012-11-30 01:54:29 +0000
> +++ lava_dispatcher/default-config/lava-dispatcher/device-defaults.conf	2012-11-30 15:53:29 +0000
> @@ -34,6 +34,11 @@
>   # XXX should be called # boot_oe_test_image_commands ?
>   boot_cmds_oe =
>
> +
> +### UEFI
> +boot_cmds_uefi =
> +### UEFI
> +

Not needed as described above in config.py

> === modified file 'lava_dispatcher/device/fastmodel.py'

Not sure why you made changes here? This looks like a no-op. I think 
they should be dropped.

> === modified file 'lava_dispatcher/device/master.py'

Now for the fun part :)

> --- lava_dispatcher/device/master.py	2012-11-30 01:57:34 +0000
> +++ lava_dispatcher/device/master.py	2012-11-30 15:53:29 +0000
> @@ -70,6 +70,7 @@
>           Target.android_deployment_data['boot_cmds'] = 'boot_cmds_android'
>           Target.ubuntu_deployment_data['boot_cmds'] = 'boot_cmds'
>           Target.oe_deployment_data['boot_cmds'] = 'boot_cmds_oe'
> +        Target.uefi_deployment_data['boot_cmds'] = 'boot_cmds_uefi'
>

might not be needed, see below

>           # used for tarballcache logic to get proper boot_cmds
>           Target.ubuntu_deployment_data['data_type'] = 'ubuntu'
> @@ -78,6 +79,7 @@
>               'android': Target.android_deployment_data,
>               'oe': Target.oe_deployment_data,
>               'ubuntu': Target.ubuntu_deployment_data,
> +            'uefi': Target.uefi_deployment_data,
>               }

might not be needed, see below

> @@ -92,6 +94,12 @@
>           self._boot_linaro_image()
>           return self.proc
>
> +### UEFI
> +    def uefi_power_on(self):
> +        self._boot_linaro_uefi_image()
> +        return self.proc
> +### UEFI
> +

Not needed see below

> @@ -104,6 +112,97 @@
>
>           self._deploy_tarballs(boot_tgz, root_tgz)
>
> +## UEFI ##
> +    def deploy_uefi(self,hwpack,rootfs):
> +        logging.info("[UEFI master.py: deploy_uefi]: %s"  % hwpack)
> +        logging.info("[UEFI master.py: deploy_uefi]: %s"  % rootfs)
> +        self.boot_master_image()
> +
> +        self._deploy_uefi_tarballs(hwpack,rootfs)

I don't think you need this function or the ones it calls. These deploy 
functions, they look identical to the current, _deploy_linaro_bootfs and 
_deploy_linaro_rootfs functions we already have in the module. On top of 
that, I don't think you even need your own deploy function. You should 
be able to use the current deploy_linaro and not even need a new action 
defined for UEFI. All you are really doing differently is setting:

  self.deployment_data = Target.uefi_deployment_data

This gets set by our currently deployment logic in:

  lava_dispatcher.device.target:Target._customize_linux

I think you could patch this function to look at boot directory - if it 
has UEFI, you could then set deployment_data to use your UEFI boot commands.

I could see a case where you might have an image with both and want to 
be able to specify which bootloader to use. In that case we already have 
a mechanism for you:

  #for your job's json file:
  {
           "command": "boot_linaro_image",
	  "parameters": {
	  	"options": ["boot_cmds=boot_cmds_uefi"]
	  }
  },