[et-mgmt-tools] [PATCH] virt-manager validation and error reporting improvements
Hugh Brock
hbrock at redhat.com
Thu Jun 21 21:04:31 UTC 2007
Cole Robinson wrote:
> Cole Robinson wrote:
>> Hi All,
>>
>> Attached is a patch that adds validation and error reporting
>> improvements to virt-manager's creation wizard. It removes redundant
>> checking and places the burden solely on virtinst, by plugging the
>> parameters into a virtinst Guest object as it goes to get the error
>> reporting and validation for free. This also simplifies the final
>> setup before beginning the install.
>>
>
> The patch didn't apply properly since my repo wasn't fully up to date.
> Here is the fixed patch.
>
> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>
This seems to work fine now, with a couple of corrections (below). I do
have one question about hanging onto the guest name, though, for which
see below...
>
> ------------------------------------------------------------------------
>
> diff -r 4df620fc9133 src/virtManager/create.py
> --- a/src/virtManager/create.py Tue Jun 19 13:20:29 2007 -0400
> +++ b/src/virtManager/create.py Mon Jun 18 15:30:35 2007 -0400
> @@ -477,97 +482,28 @@ class vmmCreate(gobject.GObject):
> return 0
>
> def finish(self, ignore=None):
> - # first things first, are we trying to create a fully virt guest?
> - if self.get_config_method() == VM_FULLY_VIRT:
> - guest = virtinst.FullVirtGuest(type=self.get_domain_type(), \
> - hypervisorURI=self.connection.get_uri(), \
> - arch=self.get_domain_arch())
> - try:
> - guest.cdrom = self.get_config_install_source()
> - except ValueError, e:
> - self._validation_error_box(_("Invalid FV media address"),e.args[0])
> - try:
> - if self.get_config_os_type() is not None and self.get_config_os_type() != "generic":
> - logging.debug("OS Type: %s" % self.get_config_os_type())
> - guest.os_type = self.get_config_os_type()
Needed to change "guest.os_type" to "self._guest.os_type" here and
below, works fine with that change.
> - except ValueError, e:
> - self._validation_error_box(_("Invalid FV OS Type"),e.args[0])
> - try:
> - if self.get_config_os_variant() is not None and self.get_config_os_type() != "generic":
> - logging.debug("OS Variant: %s" % self.get_config_os_variant())
> - guest.os_variant = self.get_config_os_variant()
> - except ValueError, e:
> - self._validation_error_box(_("Invalid FV OS Variant"),e.args[0])
> -
> - else:
> - guest = virtinst.ParaVirtGuest(type=self.get_domain_type(), hypervisorURI=self.connection.get_uri())
> - try:
> - guest.location = self.get_config_install_source()
> - except ValueError, e:
> - self._validation_error_box(_("Invalid PV media address"), e.args[0])
> - return
> - ks = self.get_config_kickstart_source()
> - if ks != None and len(ks) != 0:
> - guest.extraargs = "ks=%s" % ks
> -
> - # set the name
> + # Validation should have mostly set up out guest. We just need
> + # to take care of a few pieces we didn't touch
> +
> + guest = self._guest
> + guest.hypervisorURI = self.connection.get_uri()
> +
> + # UUID, append disk and nic
> try:
> - guest.name = self.get_config_name()
> + guest.uuid = virtinst.util.uuidToString(virtinst.util.randomUUID())
> + except ValueError, E:
> + self._validation_error_box(_("UUID Error"), str(e))
> +
> + try:
> + guest.disks.append(self._disk)
> except ValueError, e:
> - self._validation_error_box(_("Invalid system name"), e.args[0])
> - return
> -
> - # set the memory
> + self._validation_error_box(_("Error Setting up Disk"), str(e))
> +
> try:
> - guest.memory = int(self.get_config_initial_memory())
> - except ValueError:
> - self._validation_error_box(_("Invalid memory setting"), e.args[0])
> - return
> -
> - try:
> - guest.maxmemory = int(self.get_config_maximum_memory())
> - except ValueError:
> - self._validation_error_box(_("Invalid memory setting"), e.args[0])
> - return
> -
> - # set vcpus
> - guest.vcpus = int(self.get_config_virtual_cpus())
> -
> - # disks
> - filesize = None
> - if self.get_config_disk_size() != None:
> - filesize = self.get_config_disk_size() / 1024.0
> - try:
> - d = virtinst.VirtualDisk(self.get_config_disk_image(), filesize, sparse = self.is_sparse_file())
> - if d.type == virtinst.VirtualDisk.TYPE_FILE and \
> - self.get_config_method() == VM_PARA_VIRT \
> - and virtinst.util.is_blktap_capable():
> - d.driver_name = virtinst.VirtualDisk.DRIVER_TAP
> - if d.type == virtinst.VirtualDisk.TYPE_FILE and not \
> - self.is_sparse_file():
> - self.non_sparse = True
> - else:
> - self.non_sparse = False
> + guest.nics.append(self._net)
> except ValueError, e:
> - self._validation_error_box(_("Invalid storage address"), e.args[0])
> - return
> - guest.disks.append(d)
> -
> - # uuid
> - guest.uuid = virtinst.util.uuidToString(virtinst.util.randomUUID())
> -
> - # network
> - net = self.get_config_network()
> - mac = self.get_config_macaddr()
> - if net[0] == "bridge":
> - guest.nics.append(virtinst.VirtualNetworkInterface(macaddr=mac, type=net[0], bridge=net[1]))
> - elif net[0] == "network":
> - guest.nics.append(virtinst.VirtualNetworkInterface(macaddr=mac, type=net[0], network=net[1]))
> - elif net[0] == "user":
> - guest.nics.append(virtinst.VirtualNetworkInterface(macaddr=mac, type=net[0]))
> - else:
> - raise ValueError, "Unsupported networking type " + net[0]
> -
> + self._validation_error_box(_("Error Setting up Network"), str(e))
> +
> # set up the graphics to use SDL
> import keytable
> keymap = None
> @@ -597,7 +533,7 @@ class vmmCreate(gobject.GObject):
> "\n Memory: " + str(guest.memory) + \
> "\n Max Memory: " + str(guest.maxmemory) + \
> "\n # VCPUs: " + str(guest.vcpus) + \
> - "\n Filesize: " + str(filesize) + \
> + "\n Filesize: " + str(self._disk.size) + \
> "\n Disk image: " + str(self.get_config_disk_image()) +\
> "\n Non-sparse file: " + str(self.non_sparse))
>
> @@ -787,8 +723,18 @@ class vmmCreate(gobject.GObject):
> startup_mem_adjustment.upper = max_memory
>
> def validate(self, page_num):
> +
> + # Setting the values in the Guest/Disk/Network virtinst objects
> + # provides a lot of error checking for free, we just have to catch
> + # the messages
> +
> if page_num == PAGE_NAME:
> name = self.window.get_widget("create-vm-name").get_text()
> + try:
> + self._guest.name = name
> + except ValueError, e:
> + self._validation_error_box(_("Invalid System Name"), str(e))
> +
> if len(name) > 50 or len(name) == 0:
> self._validation_error_box(_("Invalid System Name"), \
> _("System name must be non-blank and less than 50 characters"))
> @@ -797,24 +743,30 @@ class vmmCreate(gobject.GObject):
> self._validation_error_box(_("Invalid System Name"), \
> _("System name may contain alphanumeric, '_' and '-' characters only"))
> return False
> -
> -
> +
> elif page_num == PAGE_TYPE:
> - if self.get_config_method() == VM_FULLY_VIRT and self.connection.get_type().startswith("Xen") and not virtinst.util.is_hvm_capable():
> - self._validation_error_box(_("Hardware Support Required"), \
> - _("Your hardware does not appear to support full virtualization. Only paravirtualized guests will be available on this hardware."))
> - return False
NIT: Seems silly to copy the entire self._guest object here merely so
you can hang onto the name. Why not just "name = self._guest.get_name()"
followed later by "self._guest.name = name"?
> +
> + # Set up appropriate guest object dependent on selected type
> + tmpguest = self._guest
> + if self.get_config_method() == VM_PARA_VIRT:
> + self._guest = virtinst.ParaVirtGuest(\
> + type=self.get_domain_type())
> + else:
> + self._guest = virtinst.FullVirtGuest(\
> + type=self.get_domain_type(), \
> + arch=self.get_domain_arch())
> +
> + self._guest.name = tmpguest.get_name() # Transfer name over
>
Otherwise looks pretty good. If you agree to the changes above I will
apply it.
Take care,
--Hugh
--
Red Hat Virtualization Group http://redhat.com/virtualization
Hugh Brock | virt-manager http://virt-manager.org
hbrock at redhat.com | virtualization library http://libvirt.org
More information about the et-mgmt-tools
mailing list