[et-mgmt-tools] [patch] virt-convert add disk signature into virt-image format export

[Cole Robinson]

Joey Boggs wrote:
> Here's what I've got, moved the hexdigest() out of the loop, cleanup and 
> more testing to verify each scenario outputs the right xml configuration 
> data.
> 
> Hope this is the final revision :)
> 
> 
> 
> 

Some comments below.

> diff -r 58a909b4f71c virt-convert
> --- a/virt-convert	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virt-convert	Wed Oct 01 17:12:45 2008 -0400
> @@ -64,6 +64,8 @@
>      opts.add_option("", "--os-variant", type="string", dest="os_variant",
>                        action="callback", callback=cli.check_before_store,
>                        help=("The OS variant for fully virtualized guests, e.g. 'fedora6', 'rhel5', 'solaris10', 'win2k', 'vista'"))
> +    opts.add_option("", "--checksum", action="store_true", dest="checksum",
> +                    help=("Generate a checksum for a virt-image guest"))
>      opts.add_option("", "--noapic", action="store_true", dest="noapic",
>          help=("Disables APIC for fully virtualized guest (overrides value in os-type/os-variant db)"), default=False)
>      opts.add_option("", "--noacpi", action="store_true", dest="noacpi",
> @@ -184,6 +186,9 @@
>  
>      unixname = vmdef.name.replace(" ", "-")
>  
> +    if options.checksum:
> +        vmdef.checksum = "yes"
> +

Rather than use a string yes/no, why not just call
the variable 'use_checksum' and have it as a bool
value?

We probably also want to add an option like 
checksum_type, since it really isn't a simple
yes/no option. If no type is specified, we can
just whatever we deem is a sensible default.
This can be worked out later though.

>      if not options.output_dir:
>          options.output_dir = unixname
>      try:
> diff -r 58a909b4f71c virtconv/parsers/virtimage.py
> --- a/virtconv/parsers/virtimage.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtconv/parsers/virtimage.py	Wed Oct 01 17:12:45 2008 -0400
> @@ -171,9 +171,41 @@
>          type = "raw"
>          if disk.type == diskcfg.DISK_TYPE_ISO:
>              type = "iso"
> -        storage.append(
> -            """<disk file="%s" use="system" format="%s"/>\n""" %
> -                (path, type))
> +
> +        if vm.checksum == "yes":
> +            md5checksum = None
> +            shachecksum = None
> +
> +            storage.append("""<disk file="%s" use="system" format="%s">\n""" % (path, type))
> +            
> +            try:
> +                import hashlib
> +                m1 = hashlib.md5(path)
> +                m2 = hashlib.sha256(path)
> +            except:
> +                import md5
> +                m1 = md5.new(path)
> +                m2 = None
> +
> +            f = open(path,"r")
> +            while 1:
> +                chunk = f.read(65536)
> +                if not chunk:
> +                    break
> +                m1.update(chunk)
> +
> +                if m2:
> +                   m2.update(chunk)
> +

I tested the above, and it generates different checksums than
the cli utils (md5sum, sha256sum). Problem seems to be that
you initialize the hash with the file's pathname. Is that
intentional?

We should probably match the output of the cli utils, so
let's make sure the hashes match for both small and large
files to be sure we aren't missing anything.

> +            md5checksum = m1.hexdigest()
> +            storage.append("""   <checksum type="md5">%s</checksum>\n""" % md5checksum)
> +
> +            if m2:
> +                shachecksum = m2.hexdigest()
> +                storage.append("""   <checksum type="sha256">%s</checksum>\n""" % shachecksum)
> +            storage.append("""   </disk>\n""")
> +        else:
> +            storage.append("""<disk file="%s" use="system" format="%s"/>\n""" % (path, type))
>  
>      return storage, diskout
>  
> diff -r 58a909b4f71c virtconv/vmcfg.py
> --- a/virtconv/vmcfg.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtconv/vmcfg.py	Wed Oct 01 17:12:45 2008 -0400
> @@ -59,6 +59,7 @@
>          self.noapic = None
>          self.os_type = None
>          self.os_variant = None
> +        self.checksum = None
>  
>      def validate(self):
>          """

Thanks,
Cole