Re: [patch] virt-convert add disk signature into virt-image format export

[Bryan Kearney <bkearney@xxxxxxxxxx>]

Cole Robinson wrote:
> Joey Boggs wrote:
> > Adds disk signatures into virt-convert for virt-image format virtual 
> > machines
>
> Comments inline.
>
> > 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	Fri Sep 26 15:58:29 2008 -0400
> > @@ -22,7 +22,7 @@
> >  import virtconv.vmcfg as vmcfg
> >  import virtconv.diskcfg as diskcfg
> >  import virtinst.FullVirtGuest as fv
> > -
> > +import sha
>
> Please keep the preceeding newline. Generally imports are 
> grouped in some attempt at a logical manner for the sake
> of readability.
>
> >  from xml.sax.saxutils import escape
> >  from string import ascii_letters
> >  import re
> > @@ -171,9 +171,11 @@
> >          type = "raw"
> >          if disk.type == diskcfg.DISK_TYPE_ISO:
> >              type = "iso"
> > +        diskfile=open(path,'r').read()
> > +        checksum=sha.new(diskfile).hexdigest()
>
> Please try to be consistent with surrounding code
> format. 
>
> The code above is using: var = val
> You've added:            var=val
>
> It helps code readability if spacing is reasonably
> consistent throughout.
>
> Hmm, so I just tried the above. What that is essentially
> trying to do is read the entire disk into memory, then
> pass it off as a giant string to the sha function. Clearly
> this is not a workable solution. This also applies to
> the virt-image hash changes as well. Haven't looked into
> the correct way to do it though.
>
> This is also a rather large performance drain. So at the
> very least it should not be the default. That said, I
> don't know the optimal way to expose this option, or
> even if we should.

I think this will become important as we look to distribute the 
appliances either via RHN or on public sites. Also.. this makes us a bit 
more consistent with OVF.

I am fine with making it off by default, and then causing the command 
line to turn it on.

-- bk

_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools