[et-mgmt-tools] [PATCH] virtinst error message improvements

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

 




Hi all,

Attached is a patch that cleans up and improves some error reporting in virtinst. Includes error checking for setting memory and vcpu, improved install location and disk location errors, and a couple other fixes.

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

Thanks,
Cole
--
Cole Robinson
crobinso@xxxxxxxxxx
diff -r 3e2bf458732a virt-install
--- a/virt-install	Thu Jun 14 08:53:12 2007 -0400
+++ b/virt-install	Sat Jun 16 14:29:36 2007 -0400
@@ -124,7 +124,11 @@ def get_vcpus(vcpus, check_cpu, guest, c
         except ValueError, e:
             print "ERROR: ", e
     if vcpus:
+        try:
             guest.vcpus = vcpus
+        except ValueError, e:
+            print "ERROR: ", e
+            
 
 def get_disk(disk, size, sparse, guest, hvm, conn):
     # FIXME: need to handle a list of disks at some point
diff -r 3e2bf458732a virtinst/DistroManager.py
--- a/virtinst/DistroManager.py	Thu Jun 14 08:53:12 2007 -0400
+++ b/virtinst/DistroManager.py	Sun Jun 17 16:17:14 2007 -0400
@@ -69,7 +69,9 @@ class URIImageFetcher(ImageFetcher):
                             text = "Verifying install location...")
             return True
         except IOError, e:
-            logging.debug("Validation failed for " + self.location + " " + str(e))
+            msg = "Opening URL " + self.location + " failed."
+            logging.debug(msg + " " + str(e))
+            raise ValueError(msg)
             return False
 
     def acquireFile(self, filename, progresscb):
@@ -117,6 +119,9 @@ class MountedImageFetcher(ImageFetcher):
         ret = subprocess.call(cmd)
         if ret != 0:
             self.cleanupLocation()
+            msg = "Mounting " + self.location + " failed."
+            logging.debug(msg)
+            raise ValueError(msg)
             return False
         return True
 
@@ -564,8 +569,11 @@ def _storeForDistro(fetcher, baseuri, ty
 # Method to fetch a krenel & initrd pair for a particular distro / HV type
 def acquireKernel(baseuri, progresscb, scratchdir="/var/tmp", type=None, distro=None):
     fetcher = _fetcherForURI(baseuri, scratchdir)
-    if not fetcher.prepareLocation(progresscb):
-        raise RuntimeError, "Invalid install location"
+    
+    try:
+        fetcher.prepareLocation(progresscb)
+    except ValueError, e:
+        raise RuntimeError, "Invalid install location: " + str(e)
 
     try:
         store = _storeForDistro(fetcher=fetcher, baseuri=baseuri, type=type, \
@@ -577,8 +585,11 @@ def acquireKernel(baseuri, progresscb, s
 # Method to fetch a bootable ISO image for a particular distro / HV type
 def acquireBootDisk(baseuri, progresscb, scratchdir="/var/tmp", type=None, distro=None):
     fetcher = _fetcherForURI(baseuri, scratchdir)
-    if not fetcher.prepareLocation(progresscb):
-        raise RuntimeError, "Invalid install location"
+
+    try:
+        fetcher.prepareLocation(progresscb)
+    except ValueError, e:
+        raise RuntimeError, "Invalid install location: " + str(e)
 
     try:
         store = _storeForDistro(fetcher=fetcher, baseuri=baseuri, type=type, \
diff -r 3e2bf458732a virtinst/Guest.py
--- a/virtinst/Guest.py	Thu Jun 14 08:53:12 2007 -0400
+++ b/virtinst/Guest.py	Sun Jun 17 16:46:08 2007 -0400
@@ -24,8 +24,6 @@ import util
 
 import logging
 
-import urlgrabber.progress as progress
-
 class VirtualDisk:
     DRIVER_FILE = "file"
     DRIVER_PHY = "phy"
@@ -51,22 +49,39 @@ class VirtualDisk:
         self.path = os.path.abspath(path)
 
         if os.path.isdir(self.path):
-            raise ValueError, "Must provide a file, not a directory for the disk"
+            raise ValueError, \
+                "The disk path must be a file/device, not a directory"
+
+        if not self.path.startswith("/"):
+            raise ValueError, \
+                "The disk path must be an absolute path location, beginning with '/'"
 
         if type is None:
             if not os.path.exists(self.path):
-                if size is None:
-                    raise ValueError, "Must provide a size for non-existent disks"
-                if size <= 0:
-                    raise ValueError, "Size of the disk image must be greater than 0"
+                logging.debug("Disk path not found: Assuming file disk type.");
                 self._type = VirtualDisk.TYPE_FILE
             else:
                 if stat.S_ISBLK(os.stat(self.path)[stat.ST_MODE]):
+                    logging.debug(\
+                        "Path is block file: Assuming Block disk type.");
                     self._type = VirtualDisk.TYPE_BLOCK
                 else:
                     self._type = VirtualDisk.TYPE_FILE
         else:
             self._type = type
+
+        if self._type == VirtualDisk.TYPE_FILE:
+            if size is None and not os.path.exists(self.path):
+                raise ValueError, \
+                    "A size must be provided for non-existent disks"
+            if size is not None and size <= 0:
+                raise ValueError, \
+                    "The size of the disk image must be greater than 0"
+        elif self._type == VirtualDisk.TYPE_BLOCK:
+            if not os.path.exists(self.path):
+                raise ValueError, "The specified block device does not exist." 
+            if not stat.S_ISBLK(os.stat(self.path)[stat.ST_MODE]):
+                raise ValueError, "The specified path is not a block device."
 
         self._readOnly = readOnly
         self._device = device
@@ -187,10 +202,11 @@ class XenDisk(VirtualDisk):
 
 class VirtualNetworkInterface:
     def __init__(self, macaddr = None, type="bridge", bridge = None, network=None):
+
         if macaddr is not None:
             form = re.match("^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$",macaddr)
             if form is None:
-                raise ValueError, "Invalid value for MAC address"
+                raise ValueError("MAC address must be of the format AA:BB:CC:DD:EE:FF")
         self.macaddr = macaddr
         self.type = type
         self.bridge = bridge
@@ -451,16 +467,14 @@ class Guest(object):
     def get_name(self):
         return self._name
     def set_name(self, val):
-        if len(val) == 0:
-            raise ValueError, "Domain name must be nonempty"
+        if len(val) > 50 or len(val) == 0:
+            raise ValueError, "System name must be greater than 0 and no more than 50 characters"
         if re.match("^[0-9]+$", val):
-            raise ValueError, "Domain name must not be numeric only"
+            raise ValueError, "System name must not be only numeric characters"
         if re.match("^[a-zA-Z0-9_-]+$", val) == None:
-            raise ValueError, "Domain name must be alphanumeric or _ or -"
-        if len(val) > 50:
-            raise ValueError, "Domain name must be less than or equal to 50 characters"
+            raise ValueError, "System name can only contain alphanumeric, '_' or '-' characters"
         if type(val) != type("string"):
-            raise ValueError, "Domain name must be a string"
+            raise ValueError, "System name must be a string"
         self._name = val
     name = property(get_name, set_name)
 
@@ -469,6 +483,8 @@ class Guest(object):
     def get_memory(self):
         return self._memory
     def set_memory(self, val):
+        if (type(val) is not type(1) or val < 0):
+            raise ValueError, "Memory value must be an integer greater than 0"
         self._memory = val
         if self._maxmemory is None or self._maxmemory < val:
             self._maxmemory = val
@@ -478,6 +494,8 @@ class Guest(object):
     def get_maxmemory(self):
         return self._maxmemory
     def set_maxmemory(self, val):
+        if (type(val) is not type(1) or val < 0):
+            raise ValueError, "Max Memory value must be an integer greater than 0"
         self._maxmemory = val
     maxmemory = property(get_maxmemory, set_maxmemory)
 
@@ -489,11 +507,15 @@ class Guest(object):
         # need better validation
         form = re.match("[a-fA-F0-9]{8}[-]([a-fA-F0-9]{4}[-]){3}[a-fA-F0-9]{12}$", val)
         if form is None:
-            form=re.match("[a-fA-F0-9]{32}$", val)
+            form = re.match("[a-fA-F0-9]{32}$", val)
             if form is None:
-                raise ValueError, "UUID must be a 32-digit hexadecimal number. It may take the form XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX or may omit hyphens altogether."
-            else:
-                val=val[0:8] + "-" + val[8:12] + "-" + val[12:16] + "-" + val[16:20] + "-" + val[20:32]
+                raise ValueError, "UUID must be a 32-digit hexadecimal " + \
+                      "number. It may take the form XXXXXXXX-XXXX-XXXX-" + \
+                      "XXXX-XXXXXXXXXXXX or may omit hyphens altogether."
+
+            else:   # UUID had no dashes, so add them in
+                val=val[0:8] + "-" + val[8:12] + "-" + val[12:16] + \
+                "-" + val[16:20] + "-" + val[20:32]
         self._uuid = val
     uuid = property(get_uuid, set_uuid)
 
@@ -502,6 +524,10 @@ class Guest(object):
     def get_vcpus(self):
         return self._vcpus
     def set_vcpus(self, val):
+        maxvcpus = util.get_max_vcpus(self.conn)
+        if val < 1 or val > get_max_vcpus:
+            raise ValueError, \
+                  "Number of vcpus must be in the range of 1-%d" % (maxvcpus,)
         self._vcpus = val
     vcpus = property(get_vcpus, set_vcpus)
 
@@ -585,10 +611,15 @@ class Guest(object):
     extraargs = property(get_extraargs, set_extraargs)
 
     def get_cdrom(self):
-        if self._installer.location is not None and self._installer.location.startswith("/"):
+        if self._installer.location is not None and \
+           self._installer.location.startswith("/"):
             return self._installer.location
         return None
     def set_cdrom(self, val):
+        if val is None or len(val) == 0:
+            raise ValueError, "You must specify an ISO location for the guest installation"
+        if not os.path.exists(val):
+            raise ValueError, "You must specify a valid path to the ISO image for guest installation"
         self._installer.location = os.path.abspath(val)
     cdrom = property(get_cdrom, set_cdrom)
 
diff -r 3e2bf458732a virtinst/util.py
--- a/virtinst/util.py	Thu Jun 14 08:53:12 2007 -0400
+++ b/virtinst/util.py	Sat Jun 16 16:12:34 2007 -0400
@@ -177,3 +177,12 @@ def get_host_network_devices():
                 if words[i] == "hwaddr":
                     device.append(words)
     return device
+
+def get_max_vcpus(conn):
+    """@conn libvirt connection to poll for max possible vcpus"""
+    try:
+        max = conn.getMaxVcpus(conn.getType())
+    except:
+        print >> stderr, "Couldn't determine max vcpus. Using 32."
+        max = 32
+    return max

[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