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<[email protected]>
> > 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
> > <[email protected]>
> >
> > ---
> > 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.
--
Onkar N Mahajan
System Software Engineer,
IBM Linux Technology Center,
Bangalore,India
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
