john.levon@xxxxxxx wrote:
Hi John,
Comments inline:
> # HG changeset patch
> # User john.levon@xxxxxxx
> # Date 1215470371 25200
> # Node ID f0ddebbf39a3926f55045ade4fbf850706ed6c3e
> # Parent c75d57437b7b219443580f6e9d6736d27d76ebcf
> Cleanup on failure
>
> If we can't convert the disks or export the file, perform some cleanup.
>
> Signed-off-by: John Levon <john.levon@xxxxxxx>
>
> diff --git a/virt-convert b/virt-convert
> --- a/virt-convert
> +++ b/virt-convert
> @@ -31,6 +31,7 @@ import virtconv.vmconfig as vmconfig
> import virtconv.vmconfig as vmconfig
>
> def parse_args():
> + """Parse and verify command line."""
> opts = OptionParser()
> opts.set_usage("%prog [options] inputdir|input.vmx "
> "[outputdir|output.xml]")
> @@ -93,6 +94,36 @@ def parse_args():
>
> return options
>
> +def rmrf(path):
> + """Remove a directory and all its contents."""
> +
> + assert path is not None
> +
> + for dirpath, _, files in os.walk(path):
You'll want to use a different variable other than '_' since this will
overwrite the gettext function in the local scope. Doesn't cause issues
in the current code, but could with future changes.
> + for filename in files:
> + os.remove(os.path.join(dirpath, filename))
> + for dirpath, subdirs, _ in os.walk(path, topdown=False):
> + for dirname in subdirs:
> + os.rmdir(os.path.join(dirpath, dirname))
> + os.rmdir(path)
> +
> +def cleanup(msg, options, created_dir):
> + """
> + After failure, clean up anything we created. Take a conservative
> + approach: only if we created the output directory do we delete
> + anything.
> + """
> + logging.error(msg)
> +
> + if created_dir:
> + try:
> + rmrf(options.output_dir)
> + except OSError, e:
> + logging.error("Couldn't clean up output directory \"%s\": %s" %
> + (options.output_dir, e.strerror))
> +
> + sys.exit(1)
> +
Hmm, did you see the comment I made in my response to this original
patch?
"Hmm, this scares the hell out of me, even with the created_dir
check. If someone in the future ever messed up this code we could
accidently start deleting someones homedir.
I think a whitelist approach would be better: have convert_disks clean
up its own files, and keep all other file/directory creation in the
main() function (like it currently is) so it can handle the rest
of the cleanup on a per file basis. At the end, if the output_dir
is empty and we created it, we can delete it. Maybe it's overly
paranoid, but better to be safe than sorry."
Any comment on this?
> def main():
> options = parse_args()
> cli.setupLogging("virt-convert", options.debug)
> @@ -131,12 +162,15 @@ def main():
>
> vmdef.arch = options.arch
>
> + created_dir = False
> unixname = vmdef.name.replace(" ", "-")
> +
> if not options.output_dir:
> options.output_dir = unixname
> try:
> logging.debug("Creating directory %s" % options.output_dir)
> os.mkdir(options.output_dir)
> + created_dir = True
> except OSError, e:
> if (e.errno != errno.EEXIST):
> logging.error("Could not create directory %s: %s" %
> @@ -156,19 +190,19 @@ def main():
> for d in vmdef.disks:
> d.convert(options.input_dir, options.output_dir,
> vmconfig.DISK_TYPE_RAW)
> - except Exception, e:
> - logging.error(e)
> - sys.exit(1)
> + except OSError, e:
> + cleanup("Couldn't convert disks: %s" % e.strerror, options, created_dir)
> + except RuntimeError, e:
> + cleanup("Couldn't convert disks: %s" % e.message, options, created_dir)
>
> try:
> outp.export_file(vmdef, options.output_file)
> - except Exception, e:
> - logging.error(e)
> - sys.exit(1)
> + except ValueError, e:
> + cleanup("Couldn't export to file \"%s\": %s" %
> + (options.output_file, e.message), options, created_dir)
>
> print "\n\nConversion completed and placed in: %s" % options.output_dir
>
> -
> if __name__ == "__main__":
> try:
> main()
> diff --git a/virtconv/vmconfig.py b/virtconv/vmconfig.py
> --- a/virtconv/vmconfig.py
> +++ b/virtconv/vmconfig.py
> @@ -77,7 +77,9 @@ class disk(object):
> (infile, qemu_formats[output_type],
> os.path.join(output_dir, outfile)))
>
> - os.system(convert_cmd)
> + ret = os.system(convert_cmd)
> + if ret != 0:
> + raise RuntimeError("qemu-img failed with exit status %d" % ret)
>
> # Note: this is the *relative* path still
> self.path = outfile
>
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools
