February 2010 - Devel - libvirt List Archives

[libvirt] [PATCH] Support VCPU hotplug in QEMU guests
by Daniel P. Berrange 16 Feb '10

16 Feb '10

QEMU has a monitor command 'set_cpu' which allows a specific CPU to be toggled between online& offline state. libvirt CPU hotplug does not work in terms of individual indexes CPUs. Thus to support this, we iteratively toggle the online state when the total number of vCPUs is adjusted via libvirt NB, currently untested since QEMU segvs when running this! * src/qemu/qemu_driver.c: Toggle online state for CPUs when doing hotplug * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h, src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Add monitor API for toggling a CPU's online status via 'set_cpu --- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_monitor.c | 14 ++++++++++ src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 44 +++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 1 + src/qemu/qemu_monitor_text.c | 38 +++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 1 + 7 files changed, 151 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 03feaf4..6ef784f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4206,6 +4206,51 @@ cleanup: } +static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int i, rc; + int ret = -1; + + /* We need different branches here, because we want to offline + * in reverse order to onlining, so any partial fail leaves us in a + * reasonably sensible state */ + if (nvcpus > vm->def->vcpus) { + for (i = vm->def->vcpus ; i < nvcpus ; i++) { + /* Online new CPU */ + rc = qemuMonitorSetCPU(priv->mon, i, 1); + if (rc == 0) + goto unsupported; + if (rc < 0) + goto cleanup; + + vm->def->vcpus++; + } + } else { + for (i = vm->def->vcpus - 1 ; i >= nvcpus ; i--) { + /* Offline old CPU */ + rc = qemuMonitorSetCPU(priv->mon, i, 0); + if (rc == 0) + goto unsupported; + if (rc < 0) + goto cleanup; + + vm->def->vcpus--; + } + } + + ret = 0; + +cleanup: + return ret; + +unsupported: + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change vcpu count of an active domain")); + goto cleanup; +} + + static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; @@ -4228,12 +4273,6 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { if (qemuDomainObjBeginJob(vm) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change vcpu count of an active domain")); - goto endjob; - } - if (!(type = virDomainVirtTypeToString(vm->def->virtType))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown virt type in domain definition '%d'"), @@ -4254,8 +4293,12 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { goto endjob; } - vm->def->vcpus = nvcpus; - ret = 0; + if (virDomainObjIsActive(vm)) { + ret = qemudDomainHotplugVcpus(vm, nvcpus); + } else { + vm->def->vcpus = nvcpus; + ret = 0; + } endjob: if (qemuDomainObjEndJob(vm) == 0) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c1d369b..541d7a1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -912,6 +912,20 @@ int qemuMonitorSetBalloon(qemuMonitorPtr mon, return ret; } + +int qemuMonitorSetCPU(qemuMonitorPtr mon, int cpu, int online) +{ + int ret; + DEBUG("mon=%p, fd=%d cpu=%d online=%d", mon, mon->fd, cpu, online); + + if (mon->json) + ret = qemuMonitorJSONSetCPU(mon, cpu, online); + else + ret = qemuMonitorTextSetCPU(mon, cpu, online); + return ret; +} + + int qemuMonitorEjectMedia(qemuMonitorPtr mon, const char *devname) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 786ad7a..d560c63 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -144,6 +144,8 @@ int qemuMonitorSetVNCPassword(qemuMonitorPtr mon, const char *password); int qemuMonitorSetBalloon(qemuMonitorPtr mon, unsigned long newmem); +int qemuMonitorSetCPU(qemuMonitorPtr mon, int cpu, int online); + /* XXX should we pass the virDomainDiskDefPtr instead * and hide devname details inside monitor. Reconsider diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c9b8d60..35e529e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -873,6 +873,50 @@ cleanup: } +/* + * Returns: 0 if CPU hotplug not supported, +1 if CPU hotplug worked + * or -1 on failure + */ +int qemuMonitorJSONSetCPU(qemuMonitorPtr mon, + int cpu, int online) +{ + int ret; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("balloon", + "U:cpu", (unsigned long long)cpu, + "s:state", online ? "online" : "offline", + NULL); + virJSONValuePtr reply = NULL; + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) { + /* XXX See if CPU soft-failed due to lack of ACPI */ +#if 0 + if (qemuMonitorJSONHasError(reply, "DeviceNotActive") || + qemuMonitorJSONHasError(reply, "KVMMissingCap")) + goto cleanup; +#endif + + /* See if any other fatal error occurred */ + ret = qemuMonitorJSONCheckError(cmd, reply); + + /* Real success */ + if (ret == 0) + ret = 1; + } + +#if 0 +cleanup: +#endif + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, const char *devname) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 65a70e3..88dfd3c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -57,6 +57,7 @@ int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon, const char *password); int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon, unsigned long newmem); +int qemuMonitorJSONSetCPU(qemuMonitorPtr mon, int cpu, int online); int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, const char *devname); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index e993699..1745b20 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -797,6 +797,44 @@ int qemuMonitorTextSetBalloon(qemuMonitorPtr mon, return ret; } + +/* + * Returns: 0 if balloon not supported, +1 if balloon adjust worked + * or -1 on failure + */ +int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, int online) +{ + char *cmd; + char *reply = NULL; + int ret = -1; + + if (virAsprintf(&cmd, "set_cpu %d %s", cpu, online ? "online" : "offline") < 0) { + virReportOOMError(NULL); + return -1; + } + + if (qemuMonitorCommand(mon, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("could nt change CPU online status")); + VIR_FREE(cmd); + return -1; + } + VIR_FREE(cmd); + + /* If the command failed qemu prints: 'unknown command' + * No message is printed on success it seems */ + if (strstr(reply, "\nunknown command:")) { + /* Don't set error - it is expected CPU onlining fails on many qemu - caller will handle */ + ret = 0; + } else { + ret = 1; + } + + VIR_FREE(reply); + return ret; +} + + int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, const char *devname) { diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 1937e99..77b8b0f 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -61,6 +61,7 @@ int qemuMonitorTextSetVNCPassword(qemuMonitorPtr mon, const char *password); int qemuMonitorTextSetBalloon(qemuMonitorPtr mon, unsigned long newmem); +int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, int online); int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, const char *devname); -- 1.6.6

3 4

Re: [libvirt] Live Migration with non-shared storage for kvm
by Kenneth Nagin 16 Feb '10

16 Feb '10

>"Daniel P. Berrange" <berrange(a)redhat.com> wrote on 26/01/2010 16:20:22: > "Daniel P. Berrange" <berrange(a)redhat.com> > 26/01/2010 16:20 > > Please respond to > "Daniel P. Berrange" <berrange(a)redhat.com> > > To > > Kenneth Nagin/Haifa/IBM@IBMIL > > cc > > Subject > > Re: [libvirt] Live Migration with non-shared storage for kvm > > On Tue, Jan 26, 2010 at 04:09:58PM +0200, Kenneth Nagin wrote: > > "Daniel P. Berrange" <berrange(a)redhat.com> wrote on 26/01/2010 12:14:47: > > Since, this my first time submitting something to libvirt I wanted to make > > the initially submission simple, i.e. just adding the flags. > > That sounds fine > > > > > > > > I suggest adding these flags to virDomainMigrate. > > > > > > That sounds reasonable. > > > > > > > If I'm not mistaken qemuMonitorTextMigrate is the function that > > actually > > > > invokes the kvm monitor. > > > > Thus, it would be necessary to pass the flags to > > qemuMonitorTextMigrate.. > > > > But qemuMonitorTextMigrate does not have a flag input parameter. I > > think > > > > the least disruptive way to support the new flags is use the existing > > > > "background" parameter > > > > to pass the flags. Of course this would require some changes to the > > > > upstream functions that are invoked for migration. > > > > > > That is an internal function, so adding extra parameters to it is no > > > problem at all - no need to reuse/abuse existing parameters. > > > > > Are you recommending adding another "flag" parameter or exploiting the > > existing "background" parameter? > > Probably define some flags for the internal API. eg in src/qemu/qemu_monitor.h > add a small enum > > enum { > QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0, > QEMU_MONITOR_MIGRATE_STORAGE = 1 << 1 > ...etc > }; > > and then just replace the existing background parameter with a flags > parameter > > > Daniel > -- > |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| > |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| Sorry about the delayed response. I had some pressing work items and family issues that took precedence. In order to support kvm's live migration with non-shared storage I added two migration flags that user can invoke: VIR_MIGRATE_NON_SHARED_DISK = (1 << 6), /* migration with non-shared storage with full disk copy */ VIR_MIGRATE_NON_SHARED_INC = (1 << 7), /* migration with non-shared storage with incremental copy */ /* (same base image shared between source and destination) */ Likewise I add complementary flags to virsh migrate: non_shared_disk and non_shared_inc As you suggested I also added internal flags to be passed in the "background" parameter to the qemu monitor: typedef enum { QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0, QEMU_MONITOR_MIGRATE_NON_SHARED_DISK = 1 << 1, /* migration with non-shared storage with full disk copy */ QEMU_MONITOR_MIGRATE_NON_SHARED_INC = 1 << 2, /* migration with non-shared storage with incremental copy */ QEMU_MONITOR_MIGRATION_FLAGS_LAST }; I updated qemu_driver.c's doNativeMigrate and doTunnelMigrate to map the external flags to the internal flags. I updated qemu_monitor_text's qemuMonitorTextMigrate to map the internal flags to the qemu monitor's command line flags. I tested the doNativeMigrate usage of the two flags. However, I could not test the doTunnelMigrate usage because I wasn't sure how it works. Also, qemudDomainSave ends up calling qemuMonitorTextMigrate, but it didn't support the external flags so I assumed that it was not relevant. Here is the diff patch file from the current git: (See attached file: migration_with_non_shared.patch) - Kenneth Nagin

1 0

[libvirt] [PATCH 0/5] macvtap support for Qemu/KVM VMs via libvirt
by Stefan Berger 16 Feb '10

16 Feb '10

Hello! This is a re-post of previously posted patches following Daniel Berrange's request for changes along with other fixes PLUS a rebase to the latest code where the conn parameter is missing along with changes requested by Daniel Veillard. The following patches provide support for making the macvtap networking device type available to Qemu/KVM VMs. The patches rely on the macvtap driver that just became available through the Linux net-next tree (fixes still may be necessary) and make the device available to Qemu/KVM via a tap file descriptor similar to a 'regular' tap device. Following up on previous discussions, the libvirt patches allow using the following XML in the domain description to enable qemu network connectivity via this type of device: <interface type='direct'> <source dev='eth1' mode='vepa'/> <model type='virtio'/> </interface> The above XML indicates that eth1 is the Ethernet interface to link the macvtap device to and communicate to the network. As a consequence, libvirt will create an instance of a macvtap device, assign it the same MAC address as the VM's interface has and open a file descriptor of the associated character device /dev/tap%d and pass it via command line to Qemu/kvm. In the above XML the mode can be chosen as 'vepa', 'private' or 'bridge' and is by default set to 'vepa'(by the driver) if omitted. Attachment and detachment of macvtap to/from a running VM also works. Regards, Stefan

3 4

[libvirt] [PATCH] libvirt-override.c: avoid a leak upon call with invalid argument
by Jim Meyering 16 Feb '10

16 Feb '10

>From 49d7a268574772d362dc374b531ff80ec87f5062 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 16 Feb 2010 08:07:38 +0100 Subject: [PATCH] libvirt-override.c: avoid a leak upon call with invalid argument * python/libvirt-override.c (libvirt_virConnectBaselineCPU): Don't leak the xmlcpus buffer upon encountering a non-string list element. --- python/libvirt-override.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index a71766a..2447ad7 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1,16 +1,16 @@ /* * libvir.c: this modules implements the main part of the glue of the * libvir library and the Python interpreter. It provides the * entry points where an automatically generated stub is * unpractical * - * Copyright (C) 2005, 2007-2009 Red Hat, Inc. + * Copyright (C) 2005, 2007-2010 Red Hat, Inc. * * Daniel Veillard <veillard(a)redhat.com> */ #include <config.h> /* Horrible kludge to work around even more horrible name-space pollution via Python.h. That file includes /usr/include/python2.5/pyconfig*.h, which has over 180 autoconf-style HAVE_* definitions. Shame on them. */ @@ -2040,20 +2040,22 @@ libvirt_virConnectBaselineCPU(PyObject *self ATTRIBUTE_UNUSED, if (PyList_Check(list)) { int i; ncpus = PyList_Size(list); if ((xmlcpus = malloc(ncpus * sizeof(*xmlcpus))) == NULL) return VIR_PY_INT_FAIL; for (i = 0; i < ncpus; i++) { xmlcpus[i] = PyString_AsString(PyList_GetItem(list, i)); - if (xmlcpus[i] == NULL) + if (xmlcpus[i] == NULL) { + free(xmlcpus); return VIR_PY_INT_FAIL; + } } } LIBVIRT_BEGIN_ALLOW_THREADS; base_cpu = virConnectBaselineCPU(conn, xmlcpus, ncpus, flags); LIBVIRT_END_ALLOW_THREADS; free(xmlcpus); -- 1.7.0.181.g41533

2 1

[libvirt] [PATCH 4/4] remote: Detect 'nc' version incompatibilities
by Cole Robinson 16 Feb '10

16 Feb '10

This ugly thing is a shell script to detect availability of the -q option for 'nc': debian and suse based distros need this flag to ensure the remote nc will exit on EOF, so it will go away when we close the tunnel. If it doesn't go away, a useless 'nc' process is left sitting on the remote host. Fedora's 'nc' doesn't have this option, so we can't blindly pass -q. More info here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=564034 We also detect the -U option, since some nc versions don't support it, which has caused quite some user confusion. Connecting to a box and using the incorrect nc: virsh --connect qemu+ssh://root@debhost/system?netcat=/bin/nc.traditional error: server closed connection: /bin/nc.traditional: invalid option -- U nc -h for help openbsd nc is required error: failed to connect to the hypervisor Test with Fedora 12, RHEL 5.4, and Debian Lenny --- src/remote/remote_driver.c | 52 ++++++++++++++++++++++++++++++++++++++------ 1 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7f92fd0..c3258d3 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -732,8 +732,49 @@ doRemoteOpen (virConnectPtr conn, } case trans_ssh: { - int j, nr_args = 6; + int j, nr_args = 0; + char *nc_command; + + const char *nc_bin = netcat ? netcat : "nc"; + const char *sock_path = (sockname ? sockname : + (flags & VIR_CONNECT_RO + ? LIBVIRTD_PRIV_UNIX_SOCKET_RO + : LIBVIRTD_PRIV_UNIX_SOCKET)); + + /* + * Build 'nc' command run on the remote host + * + * This ugly thing is a shell script to detect availability of + * the -q option for 'nc': debian and suse based distros need this + * flag to ensure the remote nc will exit on EOF, so it will go away + * when we close the tunnel. If it doesn't go away, a useless 'nc' + * process is left sitting on the remote host. + * + * Fedora's 'nc' doesn't have this option, and apparently defaults + * to the desired behavior. + */ + const char *nc_detect_template = ( + "NCOUT=$(%s -U 2>&1);" + "echo \"$NCOUT\" | grep -q 'invalid option';" + "if [ $? -eq 0 ] ; then" + " echo \"$NCOUT\" >&2;" + " echo openbsd 'nc' is required >&2;" + " exit 1;" + "fi;" + "" + "%s -q 2>&1 | grep -q 'requires an argument';" + "if [ $? -eq 0 ] ; then" + " CMD='-q 0';" + "else" + " CMD='';" + "fi;" + "%s $CMD -U %s;"); + + if (virAsprintf(&nc_command, nc_detect_template, + nc_bin, nc_bin, nc_bin, sock_path) < 0) + goto out_of_memory; + nr_args += 4; /* ssh $hostname $netcat_command NULL*/ if (username) nr_args += 2; /* For -l username */ if (no_tty) nr_args += 5; /* For -T -o BatchMode=yes -e none */ if (port) nr_args += 2; /* For -p port */ @@ -765,12 +806,9 @@ doRemoteOpen (virConnectPtr conn, cmd_argv[j++] = strdup ("none"); } cmd_argv[j++] = strdup (priv->hostname); - cmd_argv[j++] = strdup (netcat ? netcat : "nc"); - cmd_argv[j++] = strdup ("-U"); - cmd_argv[j++] = strdup (sockname ? sockname : - (flags & VIR_CONNECT_RO - ? LIBVIRTD_PRIV_UNIX_SOCKET_RO - : LIBVIRTD_PRIV_UNIX_SOCKET)); + + cmd_argv[j++] = nc_command; + cmd_argv[j++] = 0; assert (j == nr_args); for (j = 0; j < (nr_args-1); j++) -- 1.6.5.2

3 7

[libvirt] [PATCH] vboxDomainDumpXML: avoid a leak on OOM error path
by Jim Meyering 15 Feb '10

15 Feb '10

Yet another... >From 05e87caf9da26f9038935d7e54b6038b9362875b Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 15 Feb 2010 20:09:55 +0100 Subject: [PATCH] vboxDomainDumpXML: avoid a leak on OOM error path * src/vbox/vbox_tmpl.c (vboxDomainDumpXML): Free vboxCallback buffer upon OOM. --- src/vbox/vbox_tmpl.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index d1a701e..8a9af52 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5276,10 +5276,11 @@ static nsresult vboxCallbackQueryInterface(nsISupports *pThis, const nsID *iid, static IVirtualBoxCallback *vboxAllocCallbackObj(void) { IVirtualBoxCallback *vboxCallback = NULL; - /* Allocate, Initialize and return a validi + /* Allocate, Initialize and return a valid * IVirtualBoxCallback object here */ if ((VIR_ALLOC(vboxCallback) < 0) || (VIR_ALLOC(vboxCallback->vtbl) < 0)) { + VIR_FREE(vboxCallback); virReportOOMError(); return NULL; } -- 1.7.0.181.g41533

2 1

[libvirt] [PATCH] virNodeDevCapScsiHostParseXML: avoid an unconditional leak
by Jim Meyering 15 Feb '10

15 Feb '10

It looks like there may still be error-path leaks (the buffers pointed to by nodes[i]), but that can wait for another day. >From c293a97ed55c99c87bc658aaffc10831f8ba8c68 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 15 Feb 2010 19:54:45 +0100 Subject: [PATCH] virNodeDevCapScsiHostParseXML: avoid an unconditional leak * src/conf/node_device_conf.c (virNodeDevCapScsiHostParseXML): Free the "nodes" buffer allocated by virXPathNodeSet. --- src/conf/node_device_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4b65953..09c0f41 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -797,6 +797,7 @@ virNodeDevCapScsiHostParseXML(xmlXPathContextPtr ctxt, out: VIR_FREE(type); ctxt->node = orignode; + VIR_FREE(nodes); return ret; } -- 1.7.0.181.g41533

2 1

[libvirt] [PATCH] uml_driver.c: avoid leak upon failure
by Jim Meyering 15 Feb '10

15 Feb '10

Without this, it looked like every caller (there's only one; the other use is if-0'd out) could leak the result buffer. >From 8b8874b956c822562151cb90b1a8950d17b565ac Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 15 Feb 2010 19:22:38 +0100 Subject: [PATCH] uml_driver.c: avoid leak upon failure * src/uml/uml_driver.c (umlMonitorCommand): This function would sometimes return -1, yet fail to free the "reply" it had allocated. Hence, no caller would know to free the corresponding argument. When returning -1, be sure to free all allocated resources. --- src/uml/uml_driver.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 5049c92..10268db 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1,7 +1,7 @@ /* * uml_driver.c: core driver methods for managing UML guests * - * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006-2010 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -757,7 +757,10 @@ static int umlMonitorCommand(virConnectPtr conn, VIR_DEBUG("Command reply is '%s'", NULLSTR(retdata)); - *reply = retdata; + if (ret < 0) + VIR_FREE(retdata); + else + *reply = retdata; return ret; -- 1.7.0.181.g41533

2 1

[libvirt] [PATCH 2/4] qemu: Make SetVcpu command hotplug only
by Cole Robinson 15 Feb '10

15 Feb '10

Similar to the Set*Mem commands, this implementation was bogus and misleading. Make it clear this is a hotplug only operation, and that the hotplug piece isn't even implemented. Also drop the overkill maxvcpus validation: we don't perform this check at XML define time so clearly no one is missing it, and there is always the risk that our info will be out of date, possibly preventing legitimate CPU values. --- src/qemu/qemu_driver.c | 42 +++++++----------------------------------- 1 files changed, 7 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56a450c..ef1f638 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4100,12 +4100,11 @@ cleanup: } -static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { +static int qemudDomainSetVcpus(virDomainPtr dom, + ATTRIBUTE_UNUSED unsigned int nvcpus) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; - int max; int ret = -1; - const char *type; qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4119,41 +4118,14 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { goto cleanup; } - if (qemuDomainObjBeginJob(vm) < 0) + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); goto cleanup; - - if (virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change vcpu count of an active domain")); - goto endjob; - } - - if (!(type = virDomainVirtTypeToString(vm->def->virtType))) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown virt type in domain definition '%d'"), - vm->def->virtType); - goto endjob; } - if ((max = qemudGetMaxVCPUs(NULL, type)) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not determine max vcpus for the domain")); - goto endjob; - } - - if (nvcpus > max) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("requested vcpus is greater than max allowable" - " vcpus for the domain: %d > %d"), nvcpus, max); - goto endjob; - } - - vm->def->vcpus = nvcpus; - ret = 0; - -endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuReportError(VIR_ERR_NO_SUPPORT, + "%s", _("cpu hotplug not yet supported")); cleanup: if (vm) -- 1.6.5.2

2 2

[libvirt] [PATCH] openvz_driver.c: avoid leak on OOM error path
by Jim Meyering 15 Feb '10

15 Feb '10

It looks like openvzFreeDriver cannot call VIR_FREE on its buffer argument, because of the use in the function that just happens to be a few lines below. These are the only two uses of openvzFreeDriver. >From 0a616cd03cca9c9d89f23dadd52cd23c30789aca Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 15 Feb 2010 18:33:38 +0100 Subject: [PATCH] openvz_driver.c: avoid leak on OOM error path * src/openvz/openvz_driver.c (openvzOpen): Free "driver" buffer upon failure. --- src/openvz/openvz_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 68d0398..3a8a82f 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1245,22 +1245,23 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, goto cleanup; if (openvzExtractVersion(conn, driver) < 0) goto cleanup; conn->privateData = driver; return VIR_DRV_OPEN_SUCCESS; cleanup: openvzFreeDriver(driver); + VIR_FREE(driver); return VIR_DRV_OPEN_ERROR; }; static int openvzClose(virConnectPtr conn) { struct openvz_driver *driver = conn->privateData; openvzFreeDriver(driver); conn->privateData = NULL; return 0; } -- 1.7.0.181.g41533

2 3