[et-mgmt-tools] [PATCH] virtinst - virt-convert vmware output
Joey Boggs
jboggs at redhat.com
Mon Sep 29 19:28:04 UTC 2008
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>
More information about the et-mgmt-tools
mailing list