Lucas, et at., Onkar wants to add some libvirt selinux tests that require separation of on-disk resources (images, iso's, etc.) for the purposes of assigning contexts and verifying access. Part of this, for example, is testing that one running VM cannot access another running VM's resources, even when configured to do so.
The main problem is, our configuration is setup to provide shared access to resources (e.g. iso's) and offers only minimal separation and test-control over private resources (e.g. images, driver floppies, etc.). I think the main culprit is in the path mangling in tests-shared.cfg AFTER guest-os.cfg and subtests.cfg are included. I'm wondering if there's a way we can re-jigger the setup to support tests (like Onkar's) that need full control over resource locations - via the Cartesian system. Remember, all these bits were developed before we had $substitution support. I'm thinking there must be a way to leverage that so existing tests continue to work, but if/when needed some variants are able to modify the locations. Any help/advice/suggestions you could offer would be much appreciated. Thanks. On 06/27/2012 04:51 AM, Onkar N Mahajan wrote: > Hello Chris, > I have introduced some of your suggestions in the new patch. Please > talk with lmr and let me know if more changes are required ASAP so that > I can introduce those in the new patch. Please see below. > > On Tue, 2012-06-26 at 17:26 -0400, Chris Evich wrote: >> Hey, >> >> Thanks for keeping working on this and posting your work. It's normal >> for patches to go through a few rounds before getting accepted. Please >> don't take feedback in a negative way or get discourraged, we're just >> trying to help :) >> >> I think it's really cool that you want to add svirt tests and all these >> parts that go along. I have put some comments inline below. >> >> I think as this patch grows, you may consider splitting it up into a >> collection of smaller patches. If you arrange the patches in order of >> dependencies, it also helps. This is all up on the wiki very recently >> at: https://github.com/autotest/autotest/wiki/SubmissionChecklist >> >> I was happy to see that pylint didn't complain at all about your patch, >> this is good. I'm sometimes guilty of sending patches before checking, >> so don't learn from me :D >> >> On 06/26/2012 01:11 PM, Onkar N Mahajan wrote: >>> Hello Chris, >>> Please find the improved patch for adding to libvirt - support >>> for running tests with multiple VMs without interfering with >>> other VM resources. Please let me know if there are any issues- >>> my security patches are based on this - >>> >>> From 5cff67ca3f004381ddcbe862ce20ae6894bb742e Mon Sep 17 00:00:00 2001 >>> From: Onkar N Mahajan<onkar.n.maha...@linux.vnet.ibm.com> >>> Date: Tue, 26 Jun 2012 22:31:11 +0530 >>> Subject: [PATCH] Libvirt-Autotest: Support for running multiple guest >>> (Improved) Signed-off-by: Onkar N Mahajan >>> <onkar.n.maha...@linux.vnet.ibm.com> >>> >>> --- >>> client/tests/libvirt/tests-shared.cfg.sample | 12 +- >>> client/virt/libvirt_vm.py | 159 >>> +++++++++++++++++++++++++- >>> client/virt/virt_utils.py | 38 +++++-- >>> client/virt/virt_vm.py | 54 +++++++++- >>> 4 files changed, 246 insertions(+), 17 deletions(-) >>> >>> diff --git a/client/tests/libvirt/tests-shared.cfg.sample >>> b/client/tests/libvirt/tests-shared.cfg.sample >>> index 1fe0894..64cf516 100644 >>> --- a/client/tests/libvirt/tests-shared.cfg.sample >>> +++ b/client/tests/libvirt/tests-shared.cfg.sample >>> @@ -28,13 +28,13 @@ include virtio-win.cfg >>> # * The parameters cdrom_unattended, floppy, kernel and initrd are >>> generated >>> # by LIBVIRT autotest, so remember to put them under a writable >>> location >>> # (for example, the cdrom share can be read only) >>> -image_name(_.*)? ?<= /tmp/libvirt_autotest_root/images/ >>> -cdrom(_.*)? ?<= /tmp/libvirt_autotest_root/ >>> -floppy ?<= /tmp/libvirt_autotest_root/ >>> -image_dir = /tmp/libvirt_autotest_root/ >>> +image_name(_.*)? ?<= /tmp/libvirt_autotest_root/$main_vm/ >>> +cdrom(_.*)? ?<= /tmp/libvirt_autotest_root/$main_vm/ >>> +floppy ?<= /tmp/libvirt_autotest_root/$main_vm/ >>> +image_dir = /tmp/libvirt_autotest_root/$main_vm/ >>> Linux..unattended_install: >>> - kernel ?<= /tmp/libvirt_autotest_root/ >>> - initrd ?<= /tmp/libvirt_autotest_root/ >>> + kernel ?<= /tmp/libvirt_autotest_root/$main_vm/ >>> + initrd ?<= /tmp/libvirt_autotest_root/$main_vm/ >>> >>> # Uncomment the following lines to enable abort-on-error mode: >>> #abort_on_error = yes >> >> When I run the autotest "noobie" get_started.py script it still goes >> after the old directory: >> >> 15:40:56 INFO | >> 15:40:56 INFO | 3 - Verifying iso (make sure we have the OS ISO needed >> for the default test set) >> 15:40:56 INFO | Found >> /tmp/libvirt_autotest_root/isos/linux/Fedora-17-x86_64-DVD.iso >> 15:40:56 INFO | Expected SHA1 sum: 7a748072cc366ee3bdcd533afc70eda239c977c7 >> 15:40:56 INFO | Would you like to check >> /tmp/libvirt_autotest_root/isos/linux/Fedora-17-x86_64-DVD.iso? It might >> take a while (y/n) n >> 15:41:09 INFO | File >> /tmp/libvirt_autotest_root/isos/linux/Fedora-17-x86_64-DVD.iso present, >> but chose to not verify it >> >> However, when I run the cartesian parser and show contents for default >> tests, I get: >> >> # shared/cartesian_config.py tests/libvirt/tests.cfg --contents >> ... >> cdrom_cd1 = >> /tmp/libvirt_autotest_root/vm1/isos/linux/Fedora-17-x86_64-DVD.iso >> cdrom_unattended = /tmp/libvirt_autotest_root/vm1/images/f17-64/ks.iso >> ... >> > I have run this couple of times and I got it correct on every instance. > Can you check if the base_dir is correctly populated before destination > = os.path.join .... > > For me it works and looking at code I find no reason for now that it > must not work - can you please check - > > 11:22:34 DEBUG| Creating config > file /root/autotest/client/tests/libvirt/virtio-win.cfg from sample > 11:22:34 INFO | > 11:22:34 INFO | 3 - Verifying iso (make sure we have the OS ISO needed > for the default test set) >> /root/autotest/client/virt/virt_utils.py(3963)virt_test_assistant() > -> destination = os.path.join(base_dir, 'isos', 'linux') > (Pdb) p base_dir > '/tmp/libvirt_autotest_root/vm1' > (Pdb) c > 11:23:42 WARNI| > File /tmp/libvirt_autotest_root/vm1/isos/linux/Fedora-17-x86_64-DVD.iso > not found > 11:23:42 WARNI| Expected SHA1 sum: > 7a748072cc366ee3bdcd533afc70eda239c977c7 > 11:23:42 INFO | Would you like to download it from > http://download.fedoraproject.org/pub/fedora/linux/releases/17/Fedora/x86_64/iso/Fedora-17-x86_64-DVD.iso? > (y/n) > > >> This will most certainly break, unless something happens to move/link >> the cdrom image under /tmp/libvirt_autotest_root/vm1/isos/linux/... But >> if we just keep copying or moving images from the default to >> $main_vm/isos/ we'll fill up /tmp very quickly. > This is not intended - vm1's disk resources should be > under /tmp/libvirt_autotest_root/vm1/ and not > under /tmp/libvirt_autotest_root/ >> >> However, I'm guessing that you have tests which need to set specific >> selinux labels on iso images, and using bind-mount or symlinks would >> apply those labels to everything. Also consider that older libvirt and >> older anaconda can't deal with multiple cdroms (content and kickstart) >> needed for install. >> >> Another problem with current libvirt configuration setup is that the >> media/location prefixing in tests-shared.cfg happens AFTER the >> subtests.cfg where normally the test-specific variant parameters would >> live. >> > this can be dealt with later in separate patch. > >> I don't think there's a way to get this working for your tests, maintain >> the current configuration layout and not touch the unattended_install >> test - which I'm guessing that you're avoiding (I don't blame you) :D >> > This requires more code (and time) :-) > >> This is a *VERY* tricky problem to solve, while not breaking other >> tests. At the same time, I see our current setup as somewhat broken to >> start :S Rather than continue the brokenness with more patches on-top, >> maybe we can think of how to fix the setup so it works for everyone? >> > I agree with you , but we can't try all the tests for sending one patch > - this patch needs to be tried and tested - only then we will know the > drawbacks, then we can fix those problems. At this point we can see > correctness of the patch for running basic tests , as you pointed out > above in the ISO path... > >> I'd summarize the basic problem as: >> >> "How to specify a per-guest, per-OS, and per-VM image/media location >> where subtests.cfg variants may modify image/media location when needed >> (i.e. your security tests)" >> > This is tricky thing to do. Can we go ahead with this solution for now > and then gradually improve upon on need basis. > >> Maybe do these image-prefix parameters need to go into their own >> configuration module inbetween guest-os.cfg and subtest.cfg? >> >> Let's think about this more. There must be a solution that will improve >> the layout, and let your tests have per-vm private images location when >> they need them. >> >> Maybe this issue we should tackle as seperate thread? >> >> Do you have other ideas how this can be solved? >> > This is easy to do , just with introduction of a flag in guest-os.cfg > file at the time of guest configuration. I have it in my mind , but just > to keep it sweet and simple for now I purposely omitted it. The tests > which I tried - run very successfully with this patch. It is just > introducing one more directory in the path ( namely , the VM directory) > other things remain the same. > >>> diff --git a/client/virt/libvirt_vm.py b/client/virt/libvirt_vm.py >>> index d97ac0c..df9fd9b 100644 >>> --- a/client/virt/libvirt_vm.py >>> +++ b/client/virt/libvirt_vm.py >>> @@ -461,6 +461,156 @@ def virsh_detach_device(name, xml_file, extra="", >>> uri=""): >>> logging.error("Detaching device from VM %s failed." % name) >>> return False >>> >>> +def virsh_vncdisplay(name, uri=""): >>> + """ >>> + Returns the VNC display information for the domain >>> + Returns : [<IP>,<port> ] >>> + """ >>> + >>> + cmd = "vncdisplay %s" % name >>> + try: >>> + cmdresult = virsh_cmd(cmd, uri).stdout.strip() >>> + # Match IP|'':<port> >>> + m = re.search('^((?:25[0-]|2[0-4][0-9]|[01]?[0-9][0-9]?\.){3}' >>> + '(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))?\:(\d+)', >>> + cmdresult) >> >> I'm all for using regex when it makes sense, but this code is just >> matching numbers, '.' and ':'. Is regex the right tool for the job? >> > This regex matches validates IP address and port number - > 999.999.999.999 is not a valid IP address but 255.255.255.255 is ... >> I'm wondering there's a simpler way that would be a little easier on the >> eyes, and less prone to single-character typo's? If not, then doing it >> this way is okay. >> > earlier I introduced simpler regex , but that doesn't do strict IP (v4) > address validation. If you wish , I can introduce that .. > >>> + ip = m.group(1) >>> + if ip == None: >>> + ip = "0.0.0.0" >>> + vncinfo = [ ip, m.group(2)] >>> + return vncinfo >>> + except error.CmdError, details: >>> + out = str(details) >>> + if out.strip() == "": >>> + domxml = virsh_dumpxml(self.name, self.connect_uri) >>> + dom = minidom.parseString(domxml) >>> + if dom.getElementsByTagName('graphics'): >>> + display = node.childNodes[0] >>> + ap = display.attributes["autoport"] >> >> Does this work if "autoport" is not defined? IIRC it's not a required >> attribute and it's not used/supported on older libvirt versions. If you >> can, give this a try on a RHEL5 or Fedora 11 host and make sure it >> works. If not, I can double-check for you also. > > [/home/onkar/Desktop/libvirt/libvirt-0.6.2]$find . \( -name "*.py" -o > -name "*.[hc]" \) -exec grep -inH --color=auto "autoport" {} \; > ./src/domain_conf.c:1449: char *autoport; > ./src/domain_conf.c:1463: def->data.vnc.autoport = 1; > ./src/domain_conf.c:1467: def->data.vnc.autoport = 1; > ./src/domain_conf.c:1470: if ((autoport = virXMLPropString(node, > "autoport")) != NULL) { > ./src/domain_conf.c:1471: if (STREQ(autoport, "yes")) { > ./src/domain_conf.c:1474: def->data.vnc.autoport = 1; > ./src/domain_conf.c:1476: VIR_FREE(autoport); > > autoport is supported in Fedora 11, although I can add a check for > autoport attribute ... > >> >>> + if ap.value =="yes": >>> + # VNC display configured with autoport='yes' >>> + return ["0.0.0.0", -1] >>> + else: >>> + raise virt_vm.UnsupportedDisplayError(self.name, "vnc") >>> + raise virt_vm.LibvirtVirshCmdError(name, cmd, uri, details) >>> + >>> + >>> +def virsh_list(option="--all", uri = ""): >>> + """ >>> + - list domains - >>> + @param option: Can take various values see 'virsh help list' >>> + for more details. >>> + @param uri : connect uri >>> + >>> + Returns a list : >>> + Returns dictionary with each entry >>> + as :<'dom-name'>: [<dom-id> ,<dom-status> ] >>> + >>> + """ >>> + cmd = "list %s" % option >>> + try: >>> + cmd_res = virsh_cmd(cmd, uri).stdout.strip() >>> + d = {} >>> + for line in cmd_res.split("\n"): >>> + l = [] >>> + m = re.search('^\s([0-9]*\-?)\s+([0-9a-zA-Z\.-_]+)\s+(.*)', >>> line) >> >> Using regex here is fine, and right tool to use I think. I think this >> needs to be ^\s*... to match zero-or-more spaces at the beginning. i.e. >> there's room for a space and 2-digits worth of ID. But we should not >> count on how virsh decides to display ID's> 99. >> > This change is done in the new patch > >>> + if m: >>> + l.append(m.group(1)) >>> + l.append(m.group(3)) >>> + d[m.group(2)] = l >>> + return d >> >> Good, yes, I like this dictionary of id/status list building, it's easy >> to read and understand. >> >>> + except error.CmdError, details: >>> + raise virt_vm.LibvirtVirshCmdError(virsh_cmd=cmd, connuri=uri, >>> + details=details) >>> + >>> + >>> +def list_all_domains(uri, option="--all"): >> >> Since "--all" is an option, maybe method should be called 'list_domains' >> instead? But if you don't want to change the name it's okay, not a >> show-stopper. >> >>> + """ >>> + Returns list of domains for the uri >>> + """ >>> + try: >>> + val = virsh_list(option, uri) >>> + return val >>> + except virt_vm.LibvirtVirshCmdError: >>> + raise virt_vm.LibvirtFailedGettingInfoError(connuri=uri) >>> + >> >> Overall I like this new way, it's very short, simple, and to the point. >> It also matches the style of many other methods which is good. >> > thanks >>> + >>> +def get_graphics_info(name, uri, displaytype): >> >> One small thing here, I think it's okay to have the parameter default >> displaytype="vnc". IMHO "vnc" will be specified most frequently, so it's >> nice for test-writers if it's the default. >> > This is done in the new patch. >>> + """ >>> + Returns Graphics display Information configured for a given >>> + domain. >>> + ** Currently supports only VNC display ** >>> + """ >>> + if displaytype == "vnc": >>> + try: >>> + """ >>> + Add virsh functions to get graphics information >>> + here. >>> + """ >> >> Why is this here? Should it be a comment instead of a floatin string >> value? It just looks strange for some reason, maybe I had too much >> coffee :D > Comment itself can be removed safely. >>> + vncinfo = virsh_vncdisplay(name, uri) >>> + if vncinfo: >>> + ip = vncinfo[0] >>> + port = vncinfo[1] >>> + return [ip, port] >> >> Why not just return vncinfo? Is there reason to break it down, then >> build it back into list? >> > This is done in the new patch >>> + except virt_vm.UnsupportedDisplayError: >>> + raise NotImplementedError("Only VMs with VNC" >>> + "displays are supported") >>> + except virt_vm.LibvirtVirshCmdError: >>> + raise virt_vm.LibvirtFailedGettingInfoError(name, uri) >> >> I think this is fine for now. >> >>> + >>> + >>> +def get_free_port(name, uri, type, startport, endport): >> >> type = "vnc" would be nice default here too. >> > This is done in the new patch > >>> + """ >>> + Get first free port available for a given application. >>> + ** Currently supports only VNC ** >>> + Returns : port (>0) : success >>> + -1 : Failure >>> + """ >>> + if type == "vnc": >>> + val={} >>> + assigned_ports=[] >>> + maxport = 0 >>> + port = 0 >>> + try: >>> + val = list_all_domains(uri) >>> + except virt_vm.LibvirtFailedGettingInfoError: >>> + logging.error("Failed to get domains information for " >>> + "%s" % (self.connect_uri)) >>> + return -1 >>> + for key in val.keys(): >>> + try: >>> + vncinfo = get_graphics_info(key, uri, type) >>> + p = int(vncinfo[1]) >>> + if p>= startport: >>> + p = p-int(startport) >>> + if p>= maxport: >>> + maxport = p >>> + assigned_ports.append(p) >>> + except (virt_vm.LibvirtFailedGettingInfoError, >>> + NotImplementedError): >>> + logging.error("Failed to get information for VM %s " >>> + "at URI %s" % (name, uri)) >>> + return -1 >>> + cmd = "vncserver -list | grep ^:[0-9]* | cut -c1-4" >>> + cmd_result = str(utils.run(cmd)) >>> + for line in cmd_result.split("\n"): >>> + m = re.search('^:(\d)\s*$', line) >>> + if m: >>> + vncsp = m.group(1) >>> + vncserverport = int(vncsp) >>> + if vncserverport>= maxport: >>> + maxport = vncserverport >>> + assigned_ports.append(vncserverport) >>> + for p in range(0,endport-startport): >>> + if not p in assigned_ports: >>> + port = p >>> + if virt_utils.find_free_port(port,port+1): >>> + break >>> + else: >>> + continue >>> + port += startport >>> + return port >>> + >> Maybe I need MORE coffee, I think I'm not understanding something here >> :D Could you explain why we need this function? >> > > The role of this function is to choose the first available port from the > valid range of vnc ports (5900,6100). > (1) First, there will be some guests already running , if these guests > use VNC , those VNC ports must not be used for installation, > (2) Some ports are already in use by vncserver , different users uing > the host use different VNC port and these ports must also be excluded > for new guest installation > > After excluding all the above ports , whatever remains , the above > function returns least port among them in the range (5900,6100) > >> Oh! duhhhhh, nevermind, I should just scroll down :D >> ... >> >>> >>> class VM(virt_vm.BaseVM): >>> """ >>> @@ -485,6 +635,7 @@ class VM(virt_vm.BaseVM): >>> self.process = None >>> self.serial_console = None >>> self.redirs = {} >>> + self.displaytype= None >>> self.vnc_port = 5900 >>> self.vnclisten = "0.0.0.0" >>> self.pci_assignable = None >>> @@ -934,6 +1085,7 @@ class VM(virt_vm.BaseVM): >>> virt_install_cmd += add_location(help, location) >>> >>> if params.get("display") == "vnc": >>> + self.displaytype = "vnc" >>> if params.get("vnc_port"): >>> vm.vnc_port = int(params.get("vnc_port")) >>> virt_install_cmd += add_vnc(help, vm.vnc_port) >>> @@ -941,8 +1093,10 @@ class VM(virt_vm.BaseVM): >>> vm.vnclisten = params.get("vnclisten") >>> virt_install_cmd += add_vnclisten(help, vm.vnclisten) >>> elif params.get("display") == "sdl": >>> + self.displaytype = "sdl" >>> virt_install_cmd += add_sdl(help) >>> elif params.get("display") == "nographic": >>> + self.displaytype = "nographic" >>> virt_install_cmd += add_nographic(help) >>> >>> video_device = params.get("video_device") >>> @@ -1188,7 +1342,10 @@ class VM(virt_vm.BaseVM): >>> >>> # Find available VNC port, if needed >>> if params.get("display") == "vnc": >>> - self.vnc_port = virt_utils.find_free_port(5900, 6100) >>> + self.displaytype = "vnc" >>> + port = get_free_port(self.name, self.connect_uri, >>> self.displaytype, >>> + 5900, 6100) >>> + self.vnc_port = port >>> >> >> Can't we just let libvirt figure it out if there's no port? > > Current libvirt Autotest implementation does not handle this correctly > and used to fail trying to assign busy port while installation. There is > a easy way to do this - using autoport="yes", but I was thinking to > introduce some test which will use network filtering later - this will > be used there. >> IIRC if vncport == 0 libvirt behaves like autoport=true (but autoport >> isn't available on older libvirt). Maybe I'm wrong. >> > yes >>> # Find available spice port, if needed >>> if params.get("spice"): >>> diff --git a/client/virt/virt_utils.py b/client/virt/virt_utils.py >>> index 1c4d90d..5079cbb 100644 >>> --- a/client/virt/virt_utils.py >>> +++ b/client/virt/virt_utils.py >>> @@ -3889,14 +3889,36 @@ def virt_test_assistant(test_name, test_dir, >>> base_dir, default_userspace_paths, >>> logging.info("%d - Verifying directories (check if the directory >>> structure " >>> "expected by the default test config is there)", step) >>> sub_dir_list = ["images", "isos", "steps_data"] >>> - for sub_dir in sub_dir_list: >>> - sub_dir_path = os.path.join(base_dir, sub_dir) >>> - if not os.path.isdir(sub_dir_path): >>> - logging.debug("Creating %s", sub_dir_path) >>> - os.makedirs(sub_dir_path) >>> - else: >>> - logging.debug("Dir %s exists, not creating" % >>> - sub_dir_path) >>> + >>> + if test_name == 'libvirt': >>> + base_fl = "%s/%s" % (test_dir, "base.cfg") >>> + if not os.path.isfile(base_fl): >>> + base_fl = "%s/%s" % (common_dir, "base.cfg.sample") >>> + cmd = "cat %s | grep \"^main_vm\s=\s.*\" | cut -d \"=\" -f 2|sed >>> 's/[^\w]//'" % base_fl >>> + try: >>> + vmdir = utils.run("%s" % cmd).stdout >>> + except error.CmdError, detail: >>> + logging.error("Failed to fetch 'main_vm' parameter from >>> base.cfg :%s", detail) >>> + sys.exit(1) >>> + base_dir = "%s/%s" % (base_dir, vmdir.strip()) >>> + for sub_dir in sub_dir_list: >>> + sub_dir_path = os.path.join(base_dir, sub_dir) >>> + if not os.path.isdir(sub_dir_path): >>> + logging.debug("Creating %s", sub_dir_path) >>> + os.makedirs(sub_dir_path) >>> + else: >>> + logging.debug("Dir %s exists, not creating" % >>> + sub_dir_path) >>> + else: >>> + for sub_dir in sub_dir_list: >>> + sub_dir_path = os.path.join(base_dir, sub_dir) >>> + if not os.path.isdir(sub_dir_path): >>> + logging.debug("Creating %s", sub_dir_path) >>> + os.makedirs(sub_dir_path) >>> + else: >>> + logging.debug("Dir %s exists, not creating" % >>> + sub_dir_path) >>> + >>> logging.info("") >>> step += 1 >>> logging.info("%d - Creating config files from samples (copy the >>> default " >> >> This libvirt-specific stuff in the generic virt_test_assistant looks a >> little out of place. Should virt_test_assistant() maybe be broken down >> into per-vm_type sub-functions? I'm not sure how to address this. >> >> The big if test_name == 'libvirt' looks wierd to me. I'll look at it >> again tomorrow and maybe have another idea (after fresh coffee :D) >> > This is a small check in the virt_test_assistant() function - which just > introduces VM name into the path - which takes up only ~20% of the > function code - is there need for separate function ? > >>> diff --git a/client/virt/virt_vm.py b/client/virt/virt_vm.py >>> index 534f8e3..201992d 100644 >>> --- a/client/virt/virt_vm.py >>> +++ b/client/virt/virt_vm.py >>> @@ -1,8 +1,8 @@ >>> -import logging, time, glob, re >>> +import os, logging, time, glob, re, shutil, string >>> from autotest.client.shared import error >>> +from autotest.client import utils >>> import virt_utils, virt_remote >>> >>> - >>> class VMError(Exception): >>> pass >>> >>> @@ -312,6 +312,56 @@ class >>> VMUSBControllerPortFullError(VMUSBControllerError): >>> def __str__(self): >>> return ("No available USB Controller port left for VM %s." % >>> self.name) >>> >>> +class VMDisplayError(VMError): >>> + pass >>> + >>> +class UnsupportedDisplayError(VMDisplayError): >>> + def __init__(self, name, display): >>> + VMDisplayError.__init__(self, name, displaytype) >>> + self.name = name >>> + self.display = displaytype >>> + >>> + def __str__(self): >>> + return ("%s display not supported for this VM '%s' on this host." % >>> + (string.upper(self.display), self.name)) >>> + >>> +""" >>> +Libvirt error handling starts below. >>> +""" >>> + >>> +class LibvirtError(Exception): >>> + pass >>> + >>> +class LibvirtFailedGettingInfoError(LibvirtError): >>> + def __init__(self, name, connuri): >>> + LibvirtError.__init__(self, name, connuri) >>> + self.name = name >>> + self.uri = connuri >>> + >>> + def __str__(self, name=None, uri =""): >>> + if self.name: >>> + return ("Libvirt failed to get information for VM '%s' at" >>> + " connection URI '%s'" % (self.name, self.uri)) >>> + else: >>> + return ("Libvirt failed to get information for " >>> + "connection '%s' : %s" % (self.uri)) >>> + >>> +class LibvirtVirshCmdError(LibvirtError): >>> + def __init__(self, name, virsh_cmd, connuri, details=""): >>> + LibvirtError.__init__(self, name, virsh_cmd, connuri, details) >>> + self.virshcmd = virsh_cmd >>> + self.name = name >>> + self.uri = connuri >>> + self.details = details >>> + >>> + def __str__(self): >>> + if self.name : >>> + return ("Error executing command '%s' for VM '%s' at" >>> + " connection URI '%s' : " >>> + "%s" % (self.virshcmd, self.name, self.uri, >>> self.details)) >>> + else: >>> + return ("Error in executing commmand '%s' for connect URI" >>> + " %s : %s" % (self.virshcmd, self.uri, self.details)) >>> class BaseVM(object): >>> """ >> >> Is there a reason these new libvirt-specific exceptions need to be in >> the shared virt-vm module? If not, please consider moving the >> definitions into libvirt_vm. i.e. I don't think the pure KVM tests will >> ever need to raise libvirt exceptions. >> > This seems reasonable , and changed in the new patch. > >> Thanks again for this contribution, it's a lot of nice and needed code. >> I'm happy to help get it accepted, let's see if we can work out how to >> solve the configuration problem together. It's not easy problem but I >> think it needs to be fixed. I'll talk with lmr about it to see if he >> has any ideas. >> > Please talk with lmr and let me know. > -- Chris Evich, RHCA, RHCE, RHCDS, RHCSS Quality Assurance Engineer e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214 _______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
