[et-mgmt-tools] [PATCH] virtinst adding in disk signature support

[Cole Robinson]

Joey Boggs wrote:
> I reworked the else condition to raise an exception in ImageParser.py 
> rather than fail()
> 

Okay, a couple things:

> diff -r 58a909b4f71c doc/image.rng
> --- a/doc/image.rng	Mon Sep 22 11:32:11 2008 -0400
> +++ b/doc/image.rng	Tue Sep 30 10:47:20 2008 -0400
> @@ -197,6 +197,15 @@
>              </choice>
>            </attribute>
>          </optional>
> +        <optional>
> +          <element name="checksum">
> +            <attribute name="type">
> +              <value>sha1</value>
> +              <value>md5</value>
> +            </attribute>
> +            <text/>
> +          </element>
> +        </optional>
>        </element>
>      </oneOrMore>
>    </define>
> diff -r 58a909b4f71c virt-image
> --- a/virt-image	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virt-image	Tue Sep 30 10:47:20 2008 -0400
> @@ -97,6 +97,8 @@
>                        help=_("Number of vcpus to configure for your guest"))
>      parser.add_option("", "--check-cpu", action="store_true", dest="check_cpu",
>                        help=_("Check that vcpus do not exceed physical CPUs and warn if they do."))
> +    parser.add_option("", "--checksum-ignore", action="store_true", dest="checksum_ignore",
> +                      help=_("Ignore unmatching checksum values for disk signatures."))
>  
>      # network options
>      parser.add_option("-m", "--mac", type="string",
> @@ -188,6 +190,10 @@
>      # now let's get some of the common questions out of the way
>      get_name(options.name, image.name, guest)
>      get_memory(options.memory, image.domain.memory, guest)
> +
> +    if not options.checksum_ignore:
> +        cli.check_disk_signature(image,guest)
> +
>      cli.get_uuid(options.uuid, guest)
>      get_vcpus(options.vcpus, image.domain.vcpu, options.check_cpu,
>                guest, conn)
> diff -r 58a909b4f71c virtinst/ImageParser.py
> --- a/virtinst/ImageParser.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtinst/ImageParser.py	Tue Sep 30 10:47:20 2008 -0400
> @@ -213,7 +213,12 @@
>          self.format = xpathString(node, "@format", Disk.FORMAT_RAW)
>          self.size = xpathString(node, "@size")
>          self.use = xpathString(node, "@use", Disk.USE_SYSTEM)
> -
> +        self.checksum = xpathString(node, "checksum") 
> +        self.checksumtype = xpathString(node, "checksum/@type")
> +        if self.checksumtype is None or self.checksumtype == "sha1" or self.checksumtype == "md5":
> +            pass

Rather than hardcode these values in, we should do something
like CSUM_SHA1 = "sha1" etc., then stick them in a list like
the code does for formats below.

> +        else:
> +            raise ParserException("Invalid Checksum Type for %s. \n\nTo override the signature check add the --checksum-ignore option" % self.file)

This error shouldn't reference --checksum-ignore, since this
is part of the api, not tied to the command line. We also
want to mark this as translatable. I'd suggest

_("Invalid checksum type %s for file %s")

>          formats = [Disk.FORMAT_RAW, Disk.FORMAT_QCOW, Disk.FORMAT_QCOW2, Disk.FORMAT_VMDK, Disk.FORMAT_ISO]
>          validate (formats.count(self.format) > 0,
>                    _("The format for disk %s must be one of %s") %
> diff -r 58a909b4f71c virtinst/cli.py
> --- a/virtinst/cli.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtinst/cli.py	Tue Sep 30 10:47:20 2008 -0400
> @@ -30,6 +30,8 @@
>  from virtinst import Guest, CapabilitiesParser, VirtualNetworkInterface, \
>                       VirtualGraphics, VirtualAudio
>  from virtinst import _virtinst as _
> +import sha
> +import md5
>  
>  MIN_RAM = 64
>  force = False
> @@ -352,6 +354,36 @@
>      if sound:
>          guest.sound_devs.append(VirtualAudio(model="es1370"))
>

I think the following block might be better suited as part of the
Image class. It isn't really specific to cli tools, and if
anything else was built off the Image libraries they would
certainly want to have the option of using this functionality
as well.

I'd recommend moving the actual checksum calculating as
a Disk function, then add a helper to the Image class
like verify_disk_checksums or something. 
  
> +def check_disk_signature(image,guest):
> +    i = 0

The use of 'i' here is redundant. Every loop iteration,
just have a variable 'disk' that points to needs image,
no reason to stick them in a dictionary that I can see.

> +    disks = {}
> +    checksum = None
> +    for k in image.storage.keys():
> +        disks[i] = image.storage[k]
> +        
> +        if disks[i].checksumtype == "sha1":
> +            print _("\nChecking disk signature for: %s...") % disks[i].file
> +            file=open(disks[i].file,'r').read()
> +            checksum=sha.new(file).hexdigest()
> +
> +        elif disks[i].checksumtype == "md5":
> +            print _("\nChecking disk signature for: %s...") % disks[i].file
> +            file=open(disks[i].file,'r').read()
> +            checksum=md5.new(file).hexdigest()
> +        else:
> +            if disks[i].checksumtype is None: 
> +                return
> +
> +        if checksum != disks[i].checksum:
> +            fail(_("Disk signature for %s does not match \n Expected: %s \n Received: %s"
> +                   " \n\n To override the signature check add the --checksum-ignore option"
> +                   % (disks[i].file,disks[i].checksum,checksum)))
> +        else:
> +            print "Disk Signature Verified"
> +        i = i + 1    
> +
> +    return
> +
>  ### Option parsing
>  def check_before_store(option, opt_str, value, parser):
>      if len(value) == 0:

Thanks,
Cole