On Tue, Mar 01, 2011, Spring Zhang wrote:
> --- lava-dispatch	2011-03-01 03:56:20 +0000
> +++ lava-dispatch	2011-03-01 11:48:59 +0000
> -    cmd.get('parameters', {})
> +    params = cmd.get('parameters', {})

 Wups, thanks!

>      step = lava_commands[cmd['command']](target)
>      step.run(**params)
> 
> === modified file 'lava/actions/deploy.py'
> --- lava/actions/deploy.py	2011-02-26 14:41:09 +0000
> +++ lava/actions/deploy.py	2011-03-01 11:48:59 +0000
> @@ -1,33 +1,118 @@
>  #!/usr/bin/python
>  from lava.actions import BaseAction
> +from lava.client import LavaClient
>  import time
> +from subprocess import PIPE
> +from subprocess import Popen
> +import shutil
> +import shlex
>  
>  class cmd_deploy_linaro_image(BaseAction):
> +    #board type: [bootfs partition index, rootfs partition index]
> +    BOARD_PARTS = {
> +    "panda": [1, 2],
> +    "beagle": [1, 2],
> +    "vexpress": [1, 2],
> +    "mx51evk": [2, 3],

 It's a bit sad to hardcode this

 The immediate thing I would have proposed would be to have a config
 file, but in reality I'm thinking we want to work towards not knowing
 the partition layout at all.

> +        #use l-m-c to install hwpack and rootfs to qemu image
> +        #assume root permission
> +        hwpack_name = hwpack.rsplit("/", 1)[1]
> +        rootfs_name = rootfs.rsplit("/", 1)[1]
> +        qemuimg = "%s_sd.img" % boardtype
> +        lmc_cmd = "linaro-media-create --rootfs ext3 --image_file %s" \
> +                " --dev %s --binary %s --hwpack %s" % (qemuimg, boardtype, 
> +                 rootfs_name, hwpack_name)

 Hmm why do you actually need to pass just the filename to l-m-c?  Just
 pass hwpack_name and rootfs_name directly?

> +        print "l-m-c command: %s" % lmc_cmd
> +
> +        #TODO: should import l-m-c to use python way, 
> +        #      and l-m-c supports no keyboard-input
> +        """
> +        proc = Popen(shlex.split(lmc_cmd), stdin=PIPE, stdout=PIPE, 
> +                stderr=PIPE)
> +        output, rc = proc.communicate()

 You don't seem to use the output at all; use proc.wait() instead?  if
 you don't want to see it on the console, open /dev/null and pass that
 as stdout / stderr.

> +        if proc.returncode != 0:
> +            print "l-m-c fails"
> +            raise OperationFailed()

 Would be good to return a message in OperationFailed():
     if proc.returncode != 0:
         raise OperationFailed(
             "linaro-media-create failed with %s" % proc.returncode)

> +        #package qemu image content to tarballs
> +        #an ugly tar, need to find a way to create tar in relative path, 
> +        #maybe tarfile
> +        qemu_img_cmds = [ "modprobe nbd max_part=8", 
> +                "qemu-nbd --connect=/dev/nbd0 %s" % qemuimg,

 Do we need to modprovbe nbd explicitly?

> +                "mkdir -p /mnt/bootfs",
> +                "sleep 2",

 a comment on why the sleep is needed would be nice

> +                "mount /dev/nbd0p%s /mnt/bootfs" % self.BOARD_PARTS[boardtype][0],
> +                "cd /mnt/bootfs",
> +                "tar czf ~/%s ." % hwpack_name,

 tar -C to change directory; e.g. tar -C /mnt/bootfs czf foo.tgz .

 /mnt/bootfs sounds bad; you probably want to create a tmpdir instead

 ~/%s sounds bad, you probably want to use a tmp file instead

> +                "cd -",
> +                "umount /mnt/bootfs",
> +                "mkdir -p /mnt/rootfs",
> +                "mount /dev/nbd0p%s /mnt/rootfs" % self.BOARD_PARTS[boardtype][0],
> +                "cd /mnt/rootfs",
> +                "tar czf ~/%s ." % rootfs_name,
> +                "cd -",
> +                "umount /mnt/rootfs",

 ditto for /mnt/rootfs, cd/-C, and ~/%s.

> +            proc = Popen(shlex.split(cmd), stdin=PIPE, stdout=PIPE, 
> +                    stderr=PIPE)
> +            output, err = proc.communicate()

 again, output unused

> +            if proc.returncode != 0:
> +                print "Failed at command: %s" % cmd
> +                """
> +                Do more operations
> +                """
> +                raise OperationFailed()

 and should include a message in the exception when possible

> +        #move the tarball to one place of validation farm server
> +        #now it's server:/var/www/testimg
> +        shutil.copy("~/%s" % hwpack_name, "/var/www/testimg")
> +        shutil.copy("~/%s" % rootfs_name, "/var/www/testimg")

 set /var/www/testimg in a variable at the top?
 e.g. WWW_DEPLOY_DIR =

 do you actually want to copy, or would moving the tarball be ok?

 I think the rootfs pathname is going ot be the same for all boards,
 which means it will clash?

> +        #change hwpack and rootfs link to ready testimg,
> +        #however, hwpack and rootfs are not class member now
> +        hwpack = "http://pkgserver/testimg/%s" % hwpack_name
> +        rootfs = "http://pkgserver/testimg/%s" % rootfs_name

 "http://pkgserver/testimg/" sounds like it should be a constant too

>      def deploy_linaro_bootfs(self, bootfs):
> -        master_str = 'root@master:'
>          self.client.run_shell_command(
>              'mkfs.vfat /dev/disk/by-label/testboot -n testboot',
>              response = master_str)

 did you delete master_str intentionally?

 maybe it's just a missing merge

> +if __name__ == "__main__":
> +    cmd = cmd_deploy_linaro_image("bbg01")
> +    cmd.generate_tarballs("mx51evk", "http://pkgserver/original/hwpack_linaro-imx51_20110210-0_armel_unsupported.tar.gz", "http://pkgserver/original/linaro-natty-headless-tar-20110216-0.tar.gz")

 lots of hardcoding here

> +    "bbg":["mmc init",
> +        "setenv bootcmd 'fatload mmc 0:5 0x90800000 uImage; fatload mmc " \
> +        "0:5 0x90800000 uInitrd; bootm 0x90000000 0x90800000'",
> +        "setenv bootargs ' console=tty0 console=ttymxc0,115200n8 " \
> +        "root=LABEL=testrootfs rootwait ro'",
> +        "boot"],
>  }

 of course it's very sad that we have to use this, but I understand why
 you have it for now


 I'm thinking the board specific data and the server specific data could
 be isolated in a specific file with a lot of comments on what each
 constant is for

-- 
Loïc Minier