On Mon, Sep 29, 2008 at 03:28:04PM -0400, Joey Boggs wrote:

> >+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?

I just grepped and didn't see that. The only fail() usages are in
virtinst/cli.py. That's right and proper: library code like that in
virtconv/ (or most of virtinst/) should raise exceptions to allow the
caller to decide the correct behaviour (if I'm a daemon, I'd better keep
running; a GUI, I'd better bring up a dialog box, etc.).

> >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

What does this do? I had no idea that vmdk format was specific to either
SCSI or IDE - how does that work?

It's a fine restriction, but it seems inconsistent: why are we assuming
that virt-image import is using SCSI? Wouldn't a better default be IDE?

regards
john

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

John Levon wrote:

On Mon, Sep 29, 2008 at 03:28:04PM -0400, Joey Boggs wrote:



+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?



I just grepped and didn't see that. The only fail() usages are in
virtinst/cli.py. That's right and proper: library code like that in
virtconv/ (or most of virtinst/) should raise exceptions to allow the
caller to decide the correct behaviour (if I'm a daemon, I'd better keep
running; a GUI, I'd better bring up a dialog box, etc.).


Cole updated it only a few days ago, here's what I'm seeing at least:

grep -r "fail(" virt-convert

fail("Couldn't clean up output directory "%s": %s" %
fail("Couldn't import file "%s": %s" %
fail("Couldn't import file "%s": %s" % (options.input_file, e))
fail("Could not create directory %s: %s" %



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



What does this do? I had no idea that vmdk format was specific to either
SCSI or IDE - how does that work?



It's specific within the disk image file itself, not just the
configuration. I have no specifics other than that, but I believe it is
just a block tag on the device, from looking at the qemu-img vmdk scsi
support patch



It's a fine restriction, but it seems inconsistent: why are we assuming
that virt-image import is using SCSI? Wouldn't a better default be IDE?

regards
john

Never caught that, I always assumed the disks were scsi, are they not?
virt image isn't specific so I'm guessing we will need someone else to
chime in and clarify this.



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



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

On Mon, Sep 29, 2008 at 04:16:52PM -0400, Joey Boggs wrote:

> >>Not sure about this, all the other apps in virtinst use fail() now in
> >>exceptions even virt-convert, or am I misunderstanding something?
> >
> >I just grepped and didn't see that. The only fail() usages are in
> >virtinst/cli.py. That's right and proper: library code like that in
> >virtconv/ (or most of virtinst/) should raise exceptions to allow the
> >caller to decide the correct behaviour (if I'm a daemon, I'd better keep
> >running; a GUI, I'd better bring up a dialog box, etc.).
> >
> Cole updated it only a few days ago, here's what I'm seeing at least:
>
> grep -r "fail(" virt-convert
>
> fail("Couldn't clean up output directory "%s": %s" %
> fail("Couldn't import file "%s": %s" %
> fail("Couldn't import file "%s": %s" % (options.input_file, e))
> fail("Could not create directory %s: %s" %

Aren't these all in the file virt-convert? See above.

I can still write my daemon and use virtconv/ code without hitting a
fail. Your addition breaks that - is that clearer?

regards
john

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

Joey Boggs wrote:
> John Levon wrote:
>> On Mon, Sep 29, 2008 at 03:28:04PM -0400, Joey Boggs wrote:
>>
>>
>>>> +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?
>>>
>> I just grepped and didn't see that. The only fail() usages are in
>> virtinst/cli.py. That's right and proper: library code like that in
>> virtconv/ (or most of virtinst/) should raise exceptions to allow the
>> caller to decide the correct behaviour (if I'm a daemon, I'd better keep
>> running; a GUI, I'd better bring up a dialog box, etc.).
>>
> Cole updated it only a few days ago, here's what I'm seeing at least:
>
> grep -r "fail(" virt-convert
>
> fail("Couldn't clean up output directory "%s": %s" %
> fail("Couldn't import file "%s": %s" %
> fail("Couldn't import file "%s": %s" % (options.input_file, e))
> fail("Could not create directory %s: %s" %
>

'fail' should only be called from command line apps, hence you will
see it in the virt-* utils and virtinst/cli.py. The reason it
shouldn't be used anywhere else is because it forces the app to
exit. You don't want to do that from a library (aka most of
virtinst/* and all of virtconv/*) because it leaves the higher
level apps no way to deal with the error.

>
>> It's a fine restriction, but it seems inconsistent: why are we assuming
>> that virt-image import is using SCSI? Wouldn't a better default be IDE?
>>
>> regards
>> john
>>
> Never caught that, I always assumed the disks were scsi, are they not?
> virt image isn't specific so I'm guessing we will need someone else to
> chime in and clarify this.
>

If there isn't any specific mention of IDE or SCSI in the
virt-image file, then it will end up using virtinst's
default, which is IDE for fullvirt guests and xvda for
paravirt.

Thanks,
Cole

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

Got it all figured out now, its set to raise an exception rather than fail()

Also changed the bus to ide rather than scsi.




Cole Robinson wrote:

Joey Boggs wrote:


John Levon wrote:


On Mon, Sep 29, 2008 at 03:28:04PM -0400, Joey Boggs wrote:




+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?



I just grepped and didn't see that. The only fail() usages are in
virtinst/cli.py. That's right and proper: library code like that in
virtconv/ (or most of virtinst/) should raise exceptions to allow the
caller to decide the correct behaviour (if I'm a daemon, I'd better keep
running; a GUI, I'd better bring up a dialog box, etc.).



Cole updated it only a few days ago, here's what I'm seeing at least:

grep -r "fail(" virt-convert

fail("Couldn't clean up output directory "%s": %s" %
fail("Couldn't import file "%s": %s" %
fail("Couldn't import file "%s": %s" % (options.input_file, e))
fail("Could not create directory %s: %s" %




'fail' should only be called from command line apps, hence you will
see it in the virt-* utils and virtinst/cli.py. The reason it
shouldn't be used anywhere else is because it forces the app to
exit. You don't want to do that from a library (aka most of
virtinst/* and all of virtconv/*) because it leaves the higher
level apps no way to deal with the error.



It's a fine restriction, but it seems inconsistent: why are we assuming
that virt-image import is using SCSI? Wouldn't a better default be IDE?

regards
john


Never caught that, I always assumed the disks were scsi, are they not?
virt image isn't specific so I'm guessing we will need someone else to
chime in and clarify this.





If there isn't any specific mention of IDE or SCSI in the
virt-image file, then it will end up using virtinst's
default, which is IDE for fullvirt guests and xvda for
paravirt.

Thanks,
Cole




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

Joey Boggs wrote:
> Got it all figured out now, its set to raise an exception rather than fail()
>
> Also changed the bus to ide rather than scsi.
>
>

This looks mostly ready, just a few small pieces.

> diff -r 58a909b4f71c virt-convert
> --- a/virt-convert Mon Sep 22 11:32:11 2008 -0400
> +++ b/virt-convert Mon Sep 29 17:22:59 2008 -0400
> @@ -217,6 +217,8 @@
> not format and vmcfg.host() == "SunOS"):
> format = "vdisk"
>
> + elif options.output_format == "vmx":
> + format = "vmdk"
> if not format:
> format = "raw"
>
> diff -r 58a909b4f71c virtconv/diskcfg.py
> --- a/virtconv/diskcfg.py Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtconv/diskcfg.py Mon Sep 29 17:22:59 2008 -0400
> @@ -181,7 +181,7 @@
> absout = os.path.join(outdir, relout)
>
> if not (out_format == DISK_FORMAT_VDISK or
> - out_format == DISK_FORMAT_RAW):
> + out_format == DISK_FORMAT_RAW or out_format == DISK_FORMAT_VMDK):
> raise NotImplementedError("Cannot convert to disk format %s" %
> output_format)
>
> 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 17:22:59 2008 -0400
> @@ -21,10 +21,12 @@
> 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 xml.sax.saxutils import escape
> from string import ascii_letters
> +import os
> import re
>
> pv_boot_template = """
> @@ -183,16 +185,20 @@
> """
> name = "virt-image"
> suffix = ".virt-image.xml"
> - can_import = False
> + can_import = True
> can_export = True
> - can_identify = False
> + can_identify = True
>
> @staticmethod
> def identify_file(input_file):
> """
> Return True if the given file is of this format.
> """
> - raise NotImplementedError
> + try:
> + image = ImageParser.parse_file(input_file)
> + except ImageParser.ParserException, msg:
> + return False

Please log the failure here.

> + return True
>
> @staticmethod
> def import_file(input_file):
> @@ -200,7 +206,52 @@
> Import a configuration file. Raises if the file couldn't be
> opened, or parsing otherwise failed.
> """
> - raise NotImplementedError
> + vm = vmcfg.vm()
> + try:
> + config = ImageParser.parse_file(input_file)
> + except Exception, e:
> + raise Exception("Couldn't import file "%s": %s" % (input_file, e))
> +

I'd say raise a ValueError here instead of a plain
Exception. Also, just use single quotes instead of
escaping the double quotes, it's simpler.

> + domain = config.domain
> + boot = domain.boots[0]
> +
> + if not config.name:
> + raise ValueError("No Name defined in "%s"" % input_file)
> + vm.name = config.name
> + vm.memory = config.domain.memory
> + if config.descr:
> + vm.description = config.descr
> + vm.nr_vcpus = config.domain.vcpu
> +
> + bus = "ide"
> + nr_disks = 0
> + devid = (bus, nr_disks)

This line is redundant.

> +
> + 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")
> + devid = (bus, nr_disks)

You can do away with nr_disks here if you just use
len(vm.disks)

> + vm.disks[devid] = diskcfg.disk(bus = bus,
> + type = diskcfg.DISK_TYPE_DISK)
> + vm.disks[devid].format = format
> + vm.disks[devid].path = disk.file
> + nr_disks = nr_disks + 1
> +
> + nics = domain.interface
> + nr_nics = 0
> + while nr_nics < nics:
> + vm.netdevs[nr_nics] = netdevcfg.netdev(type = netdevcfg.NETDEV_TYPE_UNKNOWN)
> + nr_nics = nr_nics + 1

I'd say ditch nr_nics and just use

while nic_idx in range(0, nics):
vm.netdevs[nic_idx] ...

> + """ Eventually need to add support for mac addresses if given"""
> + vm.validate()
> + return vm
>
> @staticmethod
> def export_file(vm, output_file):
> 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 17:22:59 2008 -0400
> @@ -22,9 +22,62 @@
> import virtconv.vmcfg as vmcfg
> import virtconv.diskcfg as diskcfg
> import virtconv.netdevcfg as netdevcfg
> -
> +import time
> +import sys
> import re
> import os
> +
> +_VMX_MAIN_TEMPLATE = """
> +#!/usr/bin/vmplayer
> +
> +# Generated %(now)s by %(progname)s
> +# http://virt-manager.et.redhat.com/
> +
> +# This is a Workstation 5 or 5.5 config file and can be used with Player
> +config.version = "8"
> +virtualHW.version = "4"
> +guestOS = "other"
> +displayName = "%(vm_name)s"
> +annotation = "%(vm_description)s"
> +guestinfo.vmware.product.long = "%(vm_name)s"
> +guestinfo.vmware.product.url = "http://virt-manager.et.redhat.com/"
> +guestinfo.vmware.product.class = "virtual machine"
> +numvcpus = "%(vm_nr_vcpus)s"
> +memsize = "%(vm_memory)d"
> +MemAllowAutoScaleDown = "FALSE"
> +MemTrimRate = "-1"
> +uuid.action = "create"
> +tools.remindInstall = "TRUE"
> +hints.hideAll = "TRUE"
> +tools.syncTime = "TRUE"
> +serial0.present = "FALSE"
> +serial1.present = "FALSE"
> +parallel0.present = "FALSE"
> +logging = "TRUE"
> +log.fileName = "%(vm_name)s.log"
> +log.append = "TRUE"
> +log.keepOld = "3"
> +isolation.tools.hgfs.disable = "FALSE"
> +isolation.tools.dnd.disable = "FALSE"
> +isolation.tools.copy.enable = "TRUE"
> +isolation.tools.paste.enabled = "TRUE"
> +floppy0.present = "FALSE"
> +"""
> +_VMX_ETHERNET_TEMPLATE = """
> +ethernet%(dev)s.present = "TRUE"
> +ethernet%(dev)s.connectionType = "nat"
> +ethernet%(dev)s.addressType = "generated"
> +ethernet%(dev)s.generatedAddressOffset = "0"
> +ethernet%(dev)s.autoDetect = "TRUE"
> +"""
> +_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"
> +"""
>
> def parse_netdev_entry(vm, fullkey, value):
> """
> @@ -72,7 +125,7 @@
> # like this?
> if bus == "ide":
> inst = int(inst) + int(bus_nr) * 2
> -
> +

This is just adding trailing space.

> devid = (bus, inst)
> if not vm.disks.get(devid):
> vm.disks[devid] = diskcfg.disk(bus = bus,
> @@ -100,7 +153,7 @@
> name = "vmx"
> suffix = ".vmx"
> can_import = True
> - can_export = False
> + can_export = True
> can_identify = True
>
> @staticmethod
> @@ -160,7 +213,7 @@
> for devid, disk in vm.disks.iteritems():
> if disk.type == diskcfg.DISK_TYPE_DISK:
> continue
> -
> +
> # vmx files often have dross left in path for CD entries
> if (disk.path is None
> or disk.path.lower() == "auto detect" or
> @@ -188,7 +241,46 @@
> Raises ValueError if configuration is not suitable, or another
> exception on failure to write the output file.
> """
> + vm.description = vm.description.strip()
> + vm.description = vm.description.replace("
","|")
> + vmx_out_template = []
> + vmx_dict = {
> + "now": time.strftime("%Y-%m-%dT%H:%M:%S %Z", time.localtime()),
> + "progname": os.path.basename(sys.argv[0]),
> + "vm_name": vm.name,
> + "vm_description": vm.description or "None",
> + "vm_nr_vcpus" : vm.nr_vcpus,
> + "vm_memory": long(vm.memory)/1024
> + }
> + vmx_out = _VMX_MAIN_TEMPLATE % vmx_dict
> + vmx_out_template.append(vmx_out)
>
> - raise NotImplementedError
> + disk_out_template = []
> + diskcount = 0
> + for disk in vm.disks:
> + ide_count = 0
> + dev = "%d:%d" % (ide_count / 2, ide_count % 2)

IDE count doesn't seem to be incremented here,

> + disk_dict = {
> + "dev": dev,
> + "disk_filename" : vm.disks[disk].path
> + }
> + disk_out = _VMX_IDE_TEMPLATE % disk_dict
> + disk_out_template.append(disk_out)
> + diskcount = diskcount + 1

Diskcount isn't used.

> +
> + eth_out_template = []
> + if len(vm.netdevs):
> + for devnum in vm.netdevs:
> + eth_dict = {
> + "dev" : devnum
> + }
> + eth_out = _VMX_ETHERNET_TEMPLATE % eth_dict
> + eth_out_template.append(eth_out)
> +
> + outfile = open(output_file, "w")
> + outfile.writelines(vmx_out_template)
> + outfile.writelines(disk_out_template)
> + outfile.writelines(eth_out_template)
> + outfile.close()
>
> formats.register_parser(vmx_parser)

Thanks,
Cole

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

On Tue, Sep 30, 2008 at 01:19:57PM -0400, Cole Robinson wrote:

> > @staticmethod
> > def identify_file(input_file):
> > """
> > Return True if the given file is of this format.
> > """
> > - raise NotImplementedError
> > + try:
> > + image = ImageParser.parse_file(input_file)
> > + except ImageParser.ParserException, msg:
> > + return False
>
> Please log the failure here.

Actually, I think this is right - we try multiple identify_file()
routines until we find a sucessful one.

Perhaps this code could split out "I know this is a virt-image file"
from "I failed to parse this virt-image file", but I'm not sure it's
worth it.

regards
john

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

Yep that's why I returned as false, if we log it, it will need to be
"not recognized as a virt-image file, continuing...." or similiar



John Levon wrote:

On Tue, Sep 30, 2008 at 01:19:57PM -0400, Cole Robinson wrote:



@staticmethod
def identify_file(input_file):
"""
Return True if the given file is of this format.
"""
- raise NotImplementedError
+ try:
+ image = ImageParser.parse_file(input_file)
+ except ImageParser.ParserException, msg:
+ return False


Please log the failure here.



Actually, I think this is right - we try multiple identify_file()
routines until we find a sucessful one.

Perhaps this code could split out "I know this is a virt-image file"
from "I failed to parse this virt-image file", but I'm not sure it's
worth it.

regards
john



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

Joey Boggs wrote:
> Yep that's why I returned as false, if we log it, it will need to be
> "not recognized as a virt-image file, continuing...." or similiar
>
>

Ah, my bad. Please ignore that piece.

- Cole

> John Levon wrote:
>> On Tue, Sep 30, 2008 at 01:19:57PM -0400, Cole Robinson wrote:
>>
>>
>>>> @staticmethod
>>>> def identify_file(input_file):
>>>> """
>>>> Return True if the given file is of this format.
>>>> """
>>>> - raise NotImplementedError
>>>> + try:
>>>> + image = ImageParser.parse_file(input_file)
>>>> + except ImageParser.ParserException, msg:
>>>> + return False
>>>>
>>> Please log the failure here.
>>>
>> Actually, I think this is right - we try multiple identify_file()
>> routines until we find a sucessful one.
>>
>> Perhaps this code could split out "I know this is a virt-image file"
>> from "I failed to parse this virt-image file", but I'm not sure it's
>> worth it.
>>
>> regards
>> john
>>
>
> _______________________________________________
> et-mgmt-tools mailing list
> et-mgmt-tools@redhat.com
> https://www.redhat.com/mailman/listinfo/et-mgmt-tools

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

Made those few cleanups


Cole Robinson wrote:

Joey Boggs wrote:


Got it all figured out now, its set to raise an exception rather than fail()

Also changed the bus to ide rather than scsi.





This looks mostly ready, just a few small pieces.



diff -r 58a909b4f71c virt-convert
--- a/virt-convert Mon Sep 22 11:32:11 2008 -0400
+++ b/virt-convert Mon Sep 29 17:22:59 2008 -0400
@@ -217,6 +217,8 @@
not format and vmcfg.host() == "SunOS"):
format = "vdisk"

+ elif options.output_format == "vmx":

+ format = "vmdk"
if not format:
format = "raw"

diff -r 58a909b4f71c virtconv/diskcfg.py

--- a/virtconv/diskcfg.py Mon Sep 22 11:32:11 2008 -0400
+++ b/virtconv/diskcfg.py Mon Sep 29 17:22:59 2008 -0400
@@ -181,7 +181,7 @@
absout = os.path.join(outdir, relout)

if not (out_format == DISK_FORMAT_VDISK or

- out_format == DISK_FORMAT_RAW):
+ out_format == DISK_FORMAT_RAW or out_format == DISK_FORMAT_VMDK):
raise NotImplementedError("Cannot convert to disk format %s" %
output_format)

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 17:22:59 2008 -0400
@@ -21,10 +21,12 @@
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 xml.sax.saxutils import escape
from string import ascii_letters
+import os
import re

pv_boot_template = """

@@ -183,16 +185,20 @@
"""
name = "virt-image"
suffix = ".virt-image.xml"
- can_import = False
+ can_import = True
can_export = True
- can_identify = False
+ can_identify = True

@staticmethod

def identify_file(input_file):
"""
Return True if the given file is of this format.
"""
- raise NotImplementedError
+ try:
+ image = ImageParser.parse_file(input_file)
+ except ImageParser.ParserException, msg:
+ return False



Please log the failure here.



+ return True

@staticmethod

def import_file(input_file):
@@ -200,7 +206,52 @@
Import a configuration file. Raises if the file couldn't be
opened, or parsing otherwise failed.
"""
- raise NotImplementedError
+ vm = vmcfg.vm()
+ try:
+ config = ImageParser.parse_file(input_file)
+ except Exception, e:
+ raise Exception("Couldn't import file "%s": %s" % (input_file, e))

+



I'd say raise a ValueError here instead of a plain
Exception. Also, just use single quotes instead of
escaping the double quotes, it's simpler.



+ domain = config.domain
+ boot = domain.boots[0]
+
+ if not config.name:
+ raise ValueError("No Name defined in "%s"" % input_file)
+ vm.name = config.name
+ vm.memory = config.domain.memory
+ if config.descr:
+ vm.description = config.descr
+ vm.nr_vcpus = config.domain.vcpu
+
+ bus = "ide"
+ nr_disks = 0
+ devid = (bus, nr_disks)



This line is redundant.



+
+ 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")
+ devid = (bus, nr_disks)



You can do away with nr_disks here if you just use
len(vm.disks)



+ vm.disks[devid] = diskcfg.disk(bus = bus,
+ type = diskcfg.DISK_TYPE_DISK)
+ vm.disks[devid].format = format
+ vm.disks[devid].path = disk.file
+ nr_disks = nr_disks + 1
+
+ nics = domain.interface

+ nr_nics = 0
+ while nr_nics < nics:
+ vm.netdevs[nr_nics] = netdevcfg.netdev(type = netdevcfg.NETDEV_TYPE_UNKNOWN)
+ nr_nics = nr_nics + 1



I'd say ditch nr_nics and just use

while nic_idx in range(0, nics):
vm.netdevs[nic_idx] ...



+ """ Eventually need to add support for mac addresses if given"""
+ vm.validate()
+ return vm

@staticmethod

def export_file(vm, output_file):
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 17:22:59 2008 -0400
@@ -22,9 +22,62 @@
import virtconv.vmcfg as vmcfg
import virtconv.diskcfg as diskcfg
import virtconv.netdevcfg as netdevcfg
-
+import time
+import sys
import re
import os
+
+_VMX_MAIN_TEMPLATE = """
+#!/usr/bin/vmplayer
+
+# Generated %(now)s by %(progname)s
+# http://virt-manager.et.redhat.com/
+
+# This is a Workstation 5 or 5.5 config file and can be used with Player
+config.version = "8"
+virtualHW.version = "4"
+guestOS = "other"
+displayName = "%(vm_name)s"
+annotation = "%(vm_description)s"
+guestinfo.vmware.product.long = "%(vm_name)s"
+guestinfo.vmware.product.url = "http://virt-manager.et.redhat.com/"
+guestinfo.vmware.product.class = "virtual machine"
+numvcpus = "%(vm_nr_vcpus)s"
+memsize = "%(vm_memory)d"
+MemAllowAutoScaleDown = "FALSE"
+MemTrimRate = "-1"
+uuid.action = "create"
+tools.remindInstall = "TRUE"
+hints.hideAll = "TRUE"
+tools.syncTime = "TRUE"
+serial0.present = "FALSE"
+serial1.present = "FALSE"
+parallel0.present = "FALSE"
+logging = "TRUE"
+log.fileName = "%(vm_name)s.log"
+log.append = "TRUE"
+log.keepOld = "3"
+isolation.tools.hgfs.disable = "FALSE"
+isolation.tools.dnd.disable = "FALSE"
+isolation.tools.copy.enable = "TRUE"
+isolation.tools.paste.enabled = "TRUE"
+floppy0.present = "FALSE"
+"""
+_VMX_ETHERNET_TEMPLATE = """
+ethernet%(dev)s.present = "TRUE"
+ethernet%(dev)s.connectionType = "nat"
+ethernet%(dev)s.addressType = "generated"
+ethernet%(dev)s.generatedAddressOffset = "0"
+ethernet%(dev)s.autoDetect = "TRUE"
+"""
+_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"
+"""

def parse_netdev_entry(vm, fullkey, value):

"""
@@ -72,7 +125,7 @@
# like this?
if bus == "ide":
inst = int(inst) + int(bus_nr) * 2
-
+



This is just adding trailing space.



devid = (bus, inst)
if not vm.disks.get(devid):
vm.disks[devid] = diskcfg.disk(bus = bus,
@@ -100,7 +153,7 @@
name = "vmx"
suffix = ".vmx"
can_import = True
- can_export = False
+ can_export = True
can_identify = True

@staticmethod

@@ -160,7 +213,7 @@
for devid, disk in vm.disks.iteritems():
if disk.type == diskcfg.DISK_TYPE_DISK:
continue
-
+

# vmx files often have dross left in path for CD entries
if (disk.path is None
or disk.path.lower() == "auto detect" or
@@ -188,7 +241,46 @@
Raises ValueError if configuration is not suitable, or another
exception on failure to write the output file.
"""
+ vm.description = vm.description.strip()
+ vm.description = vm.description.replace("
","|")
+ vmx_out_template = []
+ vmx_dict = {
+ "now": time.strftime("%Y-%m-%dT%H:%M:%S %Z", time.localtime()),
+ "progname": os.path.basename(sys.argv[0]),
+ "vm_name": vm.name,
+ "vm_description": vm.description or "None",
+ "vm_nr_vcpus" : vm.nr_vcpus,
+ "vm_memory": long(vm.memory)/1024
+ }
+ vmx_out = _VMX_MAIN_TEMPLATE % vmx_dict
+ vmx_out_template.append(vmx_out)

- raise NotImplementedError

+ disk_out_template = []
+ diskcount = 0
+ for disk in vm.disks:
+ ide_count = 0
+ dev = "%d:%d" % (ide_count / 2, ide_count % 2)



IDE count doesn't seem to be incremented here,



+ disk_dict = {
+ "dev": dev,
+ "disk_filename" : vm.disks[disk].path
+ }
+ disk_out = _VMX_IDE_TEMPLATE % disk_dict
+ disk_out_template.append(disk_out)
+ diskcount = diskcount + 1



Diskcount isn't used.



+
+ eth_out_template = []
+ if len(vm.netdevs):
+ for devnum in vm.netdevs:
+ eth_dict = {
+ "dev" : devnum
+ }
+ eth_out = _VMX_ETHERNET_TEMPLATE % eth_dict
+ eth_out_template.append(eth_out)
+
+ outfile = open(output_file, "w")
+ outfile.writelines(vmx_out_template)
+ outfile.writelines(disk_out_template)
+ outfile.writelines(eth_out_template)
+ outfile.close()

formats.register_parser(vmx_parser)



Thanks,
Cole



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