Re: [PATCH] virt-manager validation and error reporting improvements

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Cole Robinson wrote:
Hugh Brock wrote:

Needed to change "guest.os_type" to "self._guest.os_type" here and below, works fine with that change.


Your comment was actually at the removed portion of the code so I was confused for a minute, but yes, definitely change that.


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


Whoops, good call.


Otherwise looks pretty good. If you agree to the changes above I will apply it.


I agree, apply at will. :)

Thanks,
Cole


Since we found some other errors, here's the (hopefully) final incarnation of this patch.

Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>

Thanks,
Cole

--
Cole Robinson
crobinso@xxxxxxxxxx
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	Tue Jun 19 09:54:31 2007 -0400
@@ -102,6 +102,11 @@ class vmmCreate(gobject.GObject):
 
         self.set_initial_state()
 
+        # Guest to fill in with values along the way
+        self._guest = virtinst.Guest(type=self.get_domain_type())
+        self._disk = None
+        self._net = None
+
     def show(self):
         self.topwin.show()
         self.reset_state()
@@ -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()
-            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,68 +723,131 @@ 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()
-            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"))
+            try:
+                self._guest.name = name
+            except ValueError, e:
+                self._validation_error_box(_("Invalid System Name"), str(e))
+
+        elif page_num == PAGE_TYPE:
+
+            # Set up appropriate guest object dependent on selected type
+            name = self._guest.name
+            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 = name # Transfer name over
+
+        elif page_num == PAGE_FVINST:
+
+            if self.window.get_widget("media-iso-image").get_active():
+
+                src = self.get_config_install_source()
+                try:
+                    self._guest.cdrom = src
+                except ValueError, e:
+                    self._validation_error_box(_("ISO Path Not Found"), str(e))
+                    return False
+            else:
+                cdlist = self.window.get_widget("cd-path")
+                src = self.get_config_install_source()
+                try:
+                    self._guest.cdrom = src
+                except ValueError, e:
+                    self._validation_error_box(_("CD-ROM Path Error"), str(e))
+                    return False
+            
+            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())
+                    self._guest.os_type = self.get_config_os_type()
+            except ValueError, e:
+                self._validation_error_box(_("Invalid FV OS Type"), str(e))
+
+            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())
+                    self._guest.os_variant = self.get_config_os_variant()
+            except ValueError, e:
+                self._validation_error_box(_("Invalid FV OS Variant"), str(e))
+
+        elif page_num == PAGE_PVINST:
+
+            src = self.get_config_install_source()
+            try:
+                self._guest.location = src
+            except ValueError, e:
+                self._validation_error_box(_("Invalid Install URL"), str(e))
                 return False
-            if re.match("^[a-zA-Z0-9_-]*$", name) == None:
-                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
-
-        elif page_num == PAGE_FVINST:
-            if self.window.get_widget("media-iso-image").get_active():
-                src = self.get_config_install_source()
-                if src == None or len(src) == 0:
-                    self._validation_error_box(_("ISO Path Required"), \
-                                               _("You must specify an ISO location for the guest installation"))
+
+            ks = self.get_config_kickstart_source()
+            if ks is not None and len(ks) != 0:
+                if not (ks.startswith("http://";) or ks.startswith("ftp://";) \
+                        or ks.startswith("nfs:")):
+                    self._validation_error_box(_("Kickstart URL Error"), \
+                                               _("Kickstart location must be an NFS, HTTP or FTP source"))
                     return False
-                elif not(os.path.exists(src)):
-                    self._validation_error_box(_("ISO Path Not Found"), \
-                                               _("You must specify a valid path to the ISO image for guest installation"))
-                    return False
-            else:
-                cdlist = self.window.get_widget("cd-path")
-                if cdlist.get_active() == -1:
-                    self._validation_error_box(_("Install media required"), \
-                                               _("You must select the CDROM install media for guest installation"))
-                    return False
-        elif page_num == PAGE_PVINST:
-            src = self.get_config_install_source()
-            if src == None or len(src) == 0:
-                self._validation_error_box(_("URL Required"), \
-                                           _("You must specify a URL for the install image for the guest install"))
-                return False
+                else:
+                    self._guest.extraargs = "ks=%s" % (ks,)
 
         elif page_num == PAGE_DISK:
+            
             disk = self.get_config_disk_image()
             if disk == None or len(disk) == 0:
                 self._validation_error_box(_("Storage Address Required"), \
                                            _("You must specify a partition or a file for storage for the guest install"))
                 return False
-
-            if not self.window.get_widget("storage-partition").get_active():
-                if os.path.isdir(disk):
-                    self._validation_error_box(_("Storage Address Is Directory"), \
-                                               _("You chose 'Simple File' storage for your storage method, but chose a directory instead of a file. Please enter a new filename or choose an existing file."))
-                    return False
-
-            d = virtinst.VirtualDisk(self.get_config_disk_image(), self.get_config_disk_size(), sparse = self.is_sparse_file())
-            if d.is_conflict_disk(self.connection.vmm) is True:
-               res = self._yes_no_box(_('Disk "%s" is already in use by another guest!' % disk), \
-                                               _("Do you really want to use the disk ?"))
+            
+            # Attempt to set disk
+            filesize = None
+            if self.get_config_disk_size() != None:
+                filesize = self.get_config_disk_size() / 1024.0
+            try:
+                if self.window.get_widget("storage-partition").get_active():
+                    type = virtinst.VirtualDisk.TYPE_BLOCK
+                else:
+                    type = virtinst.VirtualDisk.TYPE_FILE
+
+                self._disk = virtinst.VirtualDisk(\
+                                        self.get_config_disk_image(), \
+                                        filesize, \
+                                        sparse = self.is_sparse_file(), \
+                                        type=type)
+
+                if self._disk.type == virtinst.VirtualDisk.TYPE_FILE and \
+                   self.get_config_method() == VM_PARA_VIRT and \
+                   virtinst.util.is_blktap_capable():
+                    self._disk.driver_name = virtinst.VirtualDisk.DRIVER_TAP
+
+                if self._disk.type == virtinst.VirtualDisk.TYPE_FILE and not \
+                   self.is_sparse_file():
+                    self.non_sparse = True
+                else:
+                    self.non_sparse = False
+            except ValueError, e:
+                self._validation_error_box(_("Invalid Storage Address"), \
+                                            str(e))
+                return False
+
+            if self._disk.is_conflict_disk(self.connection.vmm) is True:
+               res = self._yes_no_box(_('Disk "%s" is already in use by another guest!' % disk), _("Do you really want to use the disk ?"))
                return res
 
         elif page_num == PAGE_NETWORK:
+
             if self.window.get_widget("net-type-network").get_active():
                 if self.window.get_widget("net-network").get_active() == -1:
                     self._validation_error_box(_("Virtual Network Required"),
@@ -860,28 +859,72 @@ class vmmCreate(gobject.GObject):
                                                _("You must select one of the physical devices"))
                     return False
 
+            net = self.get_config_network()
+
             if self.window.get_widget("mac-address").get_active():
                 mac = self.window.get_widget("create-mac-address").get_text()
-                if len(mac) != 17:
+                if mac is None or len(mac) == 0:
                     self._validation_error_box(_("Invalid MAC address"), \
-                                               _("MAC adrress must be 17 characters"))
+                                               _("No MAC Address was entered. Please enter a valid MAC address."))
                     return False
-                if re.match("^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$",mac) == None:
-                    self._validation_error_box(_("Invalid MAC address"), \
-                                               _("MAC address must be a form such as AA:BB:CC:DD:EE:FF, and MAC adrress may contain numeric and alphabet of A-F(a-f) and ':' characters only"))
+                try:
+                    self._guest.mac = mac
+                except ValueError, e:
+                    self._validation_error_box(_("Invalid Mac Address"), \
+                                                str(e))
                     return False
+            
                 hostdevs = virtinst.util.get_host_network_devices()
                 for hostdev in hostdevs:
                     if mac.lower() == hostdev[4]:
-                        return self._yes_no_box(_('MAC adress "%s" is already in use by host!' % mac), \
-                                                _("Do you really want to use the MAC address ?"))
-                vms = []
-                for domains in self.connection.vms.values():
-                    vms.append(domains.vm)
-                vnic = virtinst.VirtualNetworkInterface(macaddr=mac)
-                if vnic.countMACaddr(vms) > 0:
-                    return self._yes_no_box(_('MAC adress "%s" is already in use by another guest!' % mac), \
-                                            _("Do you really want to use the MAC address ?"))
+                        return self._yes_no_box(_('MAC adress "%s" is already in use by host!' % mac), _("Do you really want to use the MAC address ?"))
+            else:
+                mac = None
+            try:    
+                if net[0] == "bridge":
+                    self._net = virtinst.VirtualNetworkInterface(macaddr=mac, \
+                                                                 type=net[0], \
+                                                                 bridge=net[1])
+                elif net[0] == "network":
+                    self._net = virtinst.VirtualNetworkInterface(macaddr=mac, \
+                                                                 type=net[0], \
+                                                                 network=net[1])
+                elif net[0] == "user":
+                    self._net = virtinst.VirtualNetworkInterface(macaddr=mac, \
+                                                                 type=net[0])
+            except ValueError, e:
+                self._validation_error_box(_("Network Parameter Error"), \
+                                            str(e))
+                return False
+
+            vms = []
+            for domains in self.connection.vms.values():
+                vms.append(domains.vm)
+            if self._net.countMACaddr(vms) > 0:
+                return self._yes_no_box(_('MAC adress "%s" is already in use by another guest!' % mac), _("Do you really want to use the MAC address ?"))
+
+        elif page_num == PAGE_CPUMEM:
+
+            # Set vcpus
+            try:
+                self._guest.vcpus = int(self.get_config_virtual_cpus())
+            except ValueError, e: 
+                self._validation_error_box(_("VCPU Count Error"), \
+                                            str(e))
+
+            # Set Memory
+            try:
+                self._guest.memory = int(self.get_config_initial_memory())
+            except ValueError, e: 
+                self._validation_error_box(_("Memory Amount Error"), \
+                                            str(e))
+
+            # Set Max Memory
+            try:
+                self._guest.maxmemory = int(self.get_config_maximum_memory())
+            except ValueError, e: 
+                self._validation_error_box(_("Max Memory Amount Error"), \
+                                            str(e))
 
         # do this always, since there's no "leaving a notebook page" event.
         self.window.get_widget("create-back").set_sensitive(True)
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

[Index of Archives]     [Fedora Users]     [Fedora Legacy List]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux