[PATCH 2/2] Whitespace safety and suggested changes to commands used

Richard Maw richard.maw at codethink.co.uk
Wed Jan 2 13:31:52 GMT 2013


On Fri, Dec 21, 2012 at 11:28:06PM +0000, Jannis Pohlmann wrote:
> This command has the following syntax:
> 
>   morph deploy remote-virtualbox HOST:PATH VMNAME REPO REF SYSTEM
> 
> It resolves the REPO REF SYSTEM triplet into its artifacts, copies
> the rootfs artifact to another machine using rsync and then creates
> or updates a VirtualBox VM with the name VMNAME to boot from this
> rootfs. It makes sure that the VM is powered off before replacing
> its disk. It also starts the VM again once disks have been replaced.
> This way the process can be used to repeatedly test a built system.
> ---
>  morphlib/plugins/deployment_plugin.py | 106 ++++++++++++++++++++++++++++++----
>  1 file changed, 94 insertions(+), 12 deletions(-)
> 
> diff --git a/morphlib/plugins/deployment_plugin.py b/morphlib/plugins/deployment_plugin.py
> index 9418d0c..4ad88b9 100644
> --- a/morphlib/plugins/deployment_plugin.py
> +++ b/morphlib/plugins/deployment_plugin.py
> @@ -124,15 +124,97 @@ class DeploymentPlugin(cliapp.Plugin):
snip
> +        def run_ssh(*args, **kwargs):
> +            args = ['ssh', host] + list(args)
> +            return self.app.runcmd(args, **kwargs)
> +
> +        def run_vboxmanage(*args, **kwargs):
> +            args = ['ssh', host, 'VBoxManage'] + list(args)
> +            return self.app.runcmd(args, **kwargs)

run_vboxmanage will behave incorrectly if the args have spaces, since
ssh concatenates all the arguments given to the client, then runs them
on the remote end with system(2).

This problem would occur for uses of run_ssh as well and also requires
that commands given to run_ssh be escaped as well.

My preferred solution would be to change run_ssh to take a format string
for the shell command and arguments to interpolate, then it can escape
the arguments with something like python3's shlex module and interpolate
them into the string.

Every use of run_ssh in this patch will need to be shell escaped in some
form unless it's possible to guarantee which the paths don't need
escaping.

> +        # Make sure the VM directory exists on the host.
> +        self.app.status(msg='%(host)s: Ensuring directory "%(path)s" exists',
> +                        host=host, path=dirname)
> +        run_ssh('test -d "%s" || mkdir -p "%s"' % (dirname, dirname))

`test -d` is redundant in this case, since mkdir -p will not create the
directory, or return non-zero if it already exists.

This also means mkdir is called when the dirname is a symbolic link,
fortunately GNU coreutils' mkdir does not have problems with this.

Enclosing the string to be interpolated with " is not sufficient to keep
it safe. Using ' would mean you'd only have to escape ' characters for it
to be safe, but I would recommend complete shell escaping.

> +        # Determine image size.
> +        size = self.app.runcmd(['stat', '-c', '%s', rootfs_source]).strip()

Wouldn't it be quicker to use `os.stat` here?

> +        # 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])

See the previous whitespace issues, and since you're not using the
output of stat for anything, `test -f` can be quicker, since it can be
implemented as a shell builtin

> +        # Create the virtual machine on the host, if it doesn't already exist.
> +        self.app.status(msg='%(host)s: Checking whether VM %(vm)s exists',
> +                        host=host, vm=vm)
> +        try:
> +            state = run_ssh('VBoxManage showvminfo "%s" | '
> +                            'grep -i state' % vm).strip()
> +            vm_exists = True
> +        except:
> +            self.app.status(msg='%(host)s: Creating VM %(vm)s',
> +                            host=host, vm=vm)
> +            run_ssh('VBoxManage', 'createvm', '--name', '"%s"' % vm,
> +                    '--ostype', 'Linux26_64', '--register')
> +            vm_exists = False
> +            state = None

The naming of this variable is illogical, since it is set to False
immediately after a command has been run to create it.

It is only used in the following test, so I would remove it entirely and
change the test to whether `state` is None.

> +        if vm_exists and not 'powered off' in state:
> +            self.app.status(msg='%(host)s: Shutting down VM "%(vm)s"',
> +                            host=host, vm=vm)
> +            run_ssh('VBoxManage', 'controlvm', '"%s"' % vm, 'poweroff')
> +
> +        # Attaching the generated VDI disk to the VM as the main disk.
> +        self.app.status(msg='%(host)s: Attaching %(system)s VDI disk '
> +                            'to VM %(vm)s',
> +                        host=host, system=system, vm=vm)
> +        try:
> +            run_ssh('VBoxManage', 'storagectl', '"%s"' % vm,
> +                    '--name', '"SATA Controller"', '--add', 'sata',
> +                    '--bootable', 'on', '--sataportcount', '1', '>/dev/null')

Wouldn't it be better to keep the output of this command so that it can
be logged.

Also the fact that it can be done this way highlights that the argument
parsing of run_ssh is different from a bare runcmd. With runcmd
'>/dev/null' would be interpreted as another argument rather than a
redirection, since exec just takes an argument list.

> +        except cliapp.AppException:
> +            pass
> +        run_ssh('VBoxManage', 'storageattach', '"%s"' % vm,
> +                '--storagectl', '"SATA Controller"', '--port', '0',
> +                '--device', '0', '--type', 'hdd', '--medium',
> +                '"%s.vdi"' % rootfs_target)
> +        
> +        # Start the VM on the host.
> +        self.app.status(msg='%(host)s: Starting VM %(vm)s', host=host, vm=vm)
> +        run_ssh('VBoxManage', 'startvm', '"%s"' % vm)
> -- 
> 1.7.11.4
> 
> 
> _______________________________________________
> baserock-dev mailing list
> baserock-dev at baserock.org
> http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-baserock.org




More information about the baserock-dev mailing list