[PATCH 2/2] Add "morph deploy remote-virtualbox" command to deployment plugin

Lars Wirzenius lars.wirzenius at codethink.co.uk
Thu Jan 3 14:53:18 GMT 2013


On Fri, Dec 21, 2012 at 11:28:06PM +0000, Jannis Pohlmann wrote:
> +        # Determine image size.
> +        size = self.app.runcmd(['stat', '-c', '%s', rootfs_source]).strip()

Use os.path.getsize() instead of executing stat.

> +        # Extract the rootfs image and convert it to a VDI disk.
> +        self.app.status(msg='%(host)s: Checking if VDI %(system)s image '
> +                            'exists',
> +                        host=host, system=system)
> +        try:
> +            run_ssh(['stat', rootfs_target])
> +        except cliapp.AppException:
> +            self.app.status(msg='%(host): Converting %(system)s image to VDI',
> +                            host=host, system=system)
> +            run_ssh('gzip -c -d "%s" | '
> +                    'VBoxManage convertfromraw stdin "%s.vdi" %s; ' %
> +                    (rootfs_target, rootfs_target, rootfs_target, size))

What purpose does running the stat command on the target serve? Should
you be using os.path.exists instead?

> +        try:
> +            state = run_ssh('VBoxManage showvminfo "%s" | '
> +                            'grep -i state' % vm).strip()
> +            vm_exists = True
> +        except:

Please don't use a catch-all "except" here, or indeed, almost anywhere.
"except" catches any exception, such as syntax errors, division by
zero, keyboard interrupts. Catching these are almost never what you
want to do, and if you do, reacting to them by running commands over
ssh seems like the wrong thing to do.

Catch specific exceptions instead.

-- 
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/




More information about the baserock-dev mailing list