[et-mgmt-tools] [patch] virt-image / ImageParser disk signature verification function

[Joey Boggs]

Cole Robinson wrote:
> Joey Boggs wrote:
>   
>> This adds a new function to the virtinst.ImageParser.Disk class to 
>> verify disk signatures and runs by default when using virt-image. The 
>> next patch will wrap up loose ends in the ImageParser.Disk class to weed 
>> out unsupported checksum types. For now, if the checksum type is not 
>> matched it skips the check process.
>>
>>
>>     
>
>
>   
>> diff -r db5d9aeca590 virt-image
>> --- a/virt-image	Fri Oct 10 10:32:50 2008 -0400
>> +++ b/virt-image	Tue Oct 14 22:25:21 2008 -0400
>> @@ -197,6 +197,8 @@
>>  
>>      get_graphics(image.domain, options.vnc, options.vncport,
>>                   options.nographics, options.sdl, options.keymap, guest)
>> +    checksum = virtinst.ImageParser.Disk
>> +    checksum.check_disk_signature(image)
>>  
>>     
>
> I don't really like this interface of having a method
> of the Disk class that receives an Image object and
> pulls disks from that. Doesn't really fit the whole
> class dichotomy.
>
> The way to do it is to have the Disk method calculate
> the checksum for that particular disk. Then you can
> add a convenience method to the Image class that will
> calculate the checksums for every Disk object associated
> with that Image.
>
>   
Made all the corrections. I understand calculating for just one disk, 
which will be more reusable for other code than just this case, but why 
create separate method to check all the disks as well?

>>      if installer.is_hvm():
>>          if options.noacpi:
>> diff -r db5d9aeca590 virtinst/ImageParser.py
>> --- a/virtinst/ImageParser.py	Fri Oct 10 10:32:50 2008 -0400
>> +++ b/virtinst/ImageParser.py	Tue Oct 14 22:25:21 2008 -0400
>> @@ -23,6 +23,8 @@
>>  import libxml2
>>  import CapabilitiesParser
>>  from virtinst import _virtinst as _
>> +import logging
>> +import urlgrabber.progress as progress
>>  
>>  class ParserException(Exception):
>>      def __init__(self, msg):
>> @@ -224,6 +226,49 @@
>>                    _("The format for disk %s must be one of %s") %
>>                    (self.file, ",".join(formats)))
>>  
>> +    def check_disk_signature(self,image):
>> +        try:
>> +            import hashlib
>> +            has_hashlib = True
>> +        except:
>> +            import sha
>> +            has_hashlib = False
>> +
>> +        for disk in image.storage.values():
>> +            meter_ct = 0
>> +            m = None
>> +            disk_size = os.path.getsize(disk.id)
>>     
>
> Disk.id doesn't seem to be a required field, and
> doesn't necessarily point to a path. I would say
> use disk.file throughout.
>
>   
>> +            meter = progress.TextMeter()
>> +            meter.start(size=disk_size, text=("Checking disk signature for %s" % disk.id))
>> +
>>     
>
> Make that text=_("... to translate it.
>
>   
>> +            if has_hashlib is True:
>> +                if disk.csum.has_key("sha256"):
>> +                    csumvalue = disk.csum["sha256"]
>> +                    m = hashlib.sha256()
>> +                elif disk.csum.has_key("sha1"):
>> +                    csumvalue = disk.csum["sha1"]
>> +                    m = hashlib.sha1()
>> +            else:
>> +                if disk.csum.has_key("sha1"):
>> +                    csumvalue = disk.csum["sha1"]
>> +                    m = sha.new()   
>> +            if m:
>>     
>
> At this point, I'd just do 
>
> if not m:
>     return
>
> to avoid all the indentation.
>
>   
>> +                f = open(disk.id,"r")
>> +                while 1:
>> +                    chunk = f.read(65536)
>> +                    meter.update(meter_ct)
>> +                    meter_ct = meter_ct + 65536
>> +                    if not chunk:
>> +                        break
>>     
>
> Pop this 'if not' right after the read() so we
> don't update the meter if nothing was read.
>
>   
>> +                    m.update(chunk)
>> +                checksum = m.hexdigest()
>> +                if checksum != csumvalue:
>> +                      logging.debug(_("Disk signature for %s does not match Expected: %s  Received: %s"
>> +                                 % (disk.id,csumvalue,checksum)))
>> +                      raise ValueError (_("Disk signature for %s does not match" % disk.id))
>> +            
>> +                        
>> +
>>     
>
> Please be consistent with the current white space
> and just use 1 newline here.
>
>   
>>  def validate(cond, msg):
>>      if not cond:
>>          raise ParserException(msg)
>>     
>
>