[et-mgmt-tools] [PATCH] virtinst - virt-convert vmware output

[Joey Boggs]

comments inline, all issues addressed other than vmdk/scsi




John Levon wrote:
> On Mon, Sep 29, 2008 at 07:19:26AM -0400, Joey Boggs wrote:
>
>   
>> Made all suggested changes and cleaned up the extra whitespace
>>     
>
> Cool beans. Some comments:
>
> diff -r 58a909b4f71c virt-convert
> --- a/virt-convert	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virt-convert	Mon Sep 29 07:17:09 2008 -0400
> @@ -48,10 +48,9 @@
>      opts.add_option("-d", "--debug", action="store_true", dest="debug",
>                      help=("Print debugging information"))
>      opts.add_option("-i", "--input-format", action="store",
> -                    dest="input_format", help=("Input format, e.g. 'vmx'"))
> +                    dest="input_format", help=("Input format, e.g. 'vmx / virt-image'"))
>      opts.add_option("-o", "--output-format", action="store",
> -                    dest="output_format", default="virt-image",
> -                    help=("Output format, e.g. 'virt-image'"))
> +                    dest="output_format", help=("Output format, e.g. 'virt-image / vmx'"))
>
> These are just examples, so why list all the formats? The existing ones
> should be fine.
>
>   
reverting changes
> 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	Mon Sep 29 07:17:09 2008 -0400
> @@ -21,10 +21,13 @@
>  import virtconv.formats as formats
>  import virtconv.vmcfg as vmcfg
>  import virtconv.diskcfg as diskcfg
> +import virtconv.netdevcfg as netdevcfg
>  import virtinst.FullVirtGuest as fv
> -
> +import virtinst.ImageParser as ImageParser
> +from virtinst.cli import fail
>
> Surely this isn't right - this code is "library" code and shouldn't be
> using fail() ? It should be re-raising the exception...
>   

Not sure about this, all the other apps in virtinst use fail() now in 
exceptions even virt-convert, or am I misunderstanding something?
> +        bus="scsi"
>
> Inconsistent spacing.
>
> +        diskinst = 0
>
> Isn't nr_disks a little more understandable?
>
> +        devid = (bus, diskinst)
>
> This line...
>
> +
> +        for d in boot.drives:
> +            disk = d.disk
> +            format = None
> +            if disk.format == ImageParser.Disk.FORMAT_RAW:
> +                format = diskcfg.DISK_FORMAT_RAW
> +            elif format == ImageParser.DISK_FORMAT_VMDK:
> +                format == diskcfg.DISK_FORMAT_VMDK
> +            
> +            if format is None:
> +                raise ValueError("Unable to determine disk format")
> +            
>
> ... should surely be here?
>
> +            vm.disks[devid] = diskcfg.disk(bus = bus,
> +                type = diskcfg.DISK_TYPE_DISK)
>
> No CD-ROM handling?
>
> +        nics = domain.interface
> +        nicinst = 0
>
> nr_nics? :)
>
>  
> diff -r 58a909b4f71c virtconv/parsers/vmx.py
> --- a/virtconv/parsers/vmx.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtconv/parsers/vmx.py	Mon Sep 29 07:17:09 2008 -0400
> +_VMX_IDE_TEMPLATE = """
> +# IDE disk
> +ide%(dev)s.present = "TRUE"
> +ide%(dev)s.fileName = "%(disk_filename)s"
> +ide%(dev)s.mode = "persistent"
> +ide%(dev)s.startConnected = "TRUE"
> +ide%(dev)s.writeThrough = "TRUE"
> +"""
>
> Hmm, above we're importing virt-image as SCSI disks, but exporting as
> IDE - can you clarify this?
>   
We can't export as scsi without qemu-img vmdk scsi support. It's in the 
wild and in opensuse but not upstream nor Fedora. I'll file a bugzilla 
on our side for it. When we add more formats the vmx ide exception will 
impact them if we change how they are imported on the virt-image side. 
Can't we import them and export them as we have available? Once we get 
both ide/scsi vmdk support we can identify and export in either needed 
format.

>  
>  def parse_netdev_entry(vm, fullkey, value):
>      """
> @@ -70,14 +123,13 @@
>  
>      # Does anyone else think it's scary that we're still doing things
>      # like this?
> +   
>      if bus == "ide":
>          inst = int(inst) + int(bus_nr) * 2
> -
>      devid = (bus, inst)
>      if not vm.disks.get(devid):
>          vm.disks[devid] = diskcfg.disk(bus = bus,
>              type = diskcfg.DISK_TYPE_DISK)
> -
>      if key == "devicetype":
>          if lvalue == "atapi-cdrom" or lvalue == "cdrom-raw":
>              vm.disks[devid].type = diskcfg.DISK_TYPE_CDROM
>
> I don't see these whitespace changes as improvements...
>
> @@ -180,6 +232,8 @@
>  
>      @staticmethod
>      def export_file(vm, output_file):
> +
> +
>
> ditto.
>
> @@ -188,7 +242,47 @@
>          Raises ValueError if configuration is not suitable, or another
>          exception on failure to write the output file.
>          """
> +        vmx_dict = {
> +            "now": time.strftime("%Y-%m-%dT%H:%M:%S %Z", time.localtime()),
> +            "progname": os.path.basename(sys.argv[0]),
> +            "/image/name": vm.name,
> +            "/image/description": vm.description or "None",
> +            "/image/label": vm.name,
> +            "/image/devices/vcpu" : vm.nr_vcpus,
> +            "/image/devices/memory": long(vm.memory)/1024
>
> It's not clear why you used such strange names for the dict's keywords?
>   
that was copy/past from virt-pack, all updated to new values
> regards
> john
>
> _______________________________________________
> et-mgmt-tools mailing list
> et-mgmt-tools at redhat.com
> https://www.redhat.com/mailman/listinfo/et-mgmt-tools
>   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: virt-convert-vmware-conversion-module-9-29-1522.patch
Type: text/x-patch
Size: 8408 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/et-mgmt-tools/attachments/20080929/ab2d4955/attachment.bin>