- Devel - libvirt List Archives

[libvirt] FYI, I've pushed a bunch of ACK'd patches
by Jim Meyering 18 May '10

18 May '10

I've pushed these: ebiptablesWriteToTempFile: don't close a negative file descriptor virDomainNetDefParseXML: avoid leak upon multiple "filterref" linuxNodeInfoCPUPopulate: avoid used-uninitialized via a test virNWFilterDefParseXML: avoid leak on error paths qemuMonitorTextMigrate: avoid leak on OOM-error path tests: do not ignore virInitialize failure python: don't ignore virInitialize failure in module initialization qemudDomainRestore: handle a case of virDomainSaveStatus failure ebtablesAddRemoveRule, iptablesAddRemoveRule: don't skip va_end do not ignore qemuMonitorAddDrive failure; make uses identical

1 0

[libvirt] [PATCH] virDomainNetDefParseXML: avoid leak upon multiple "filterref"
by Jim Meyering 18 May '10

18 May '10

The offending code below appears in this loop: virNWFilterHashTablePtr filterparams = NULL; ... cur = node->children; while (cur != NULL) { ... } so the first assignment works fine, but second and subsequent ones leak the buffer returned by each preceding virNWFilterParseParamAttributes call. >From 8659fb1ae879befe360e1ec7b8b62434c22698cd Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 18 May 2010 10:19:30 +0200 Subject: [PATCH] virDomainNetDefParseXML: avoid leak upon multiple "filterref" * src/conf/domain_conf.c (virDomainNetDefParseXML): Don't leak memory when parsing two or more "filterref" elements. --- src/conf/domain_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e45f79..0c717f2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1905,6 +1905,7 @@ virDomainNetDefParseXML(virCapsPtr caps, model = virXMLPropString(cur, "type"); } else if (xmlStrEqual (cur->name, BAD_CAST "filterref")) { filter = virXMLPropString(cur, "filter"); + free(filterparams); filterparams = virNWFilterParseParamAttributes(cur); } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { -- 1.7.1.250.g7d1e8

3 3

[libvirt] [PATCH] tests: do not ignore virInitialize failure
by Jim Meyering 18 May '10

18 May '10

Simple... >From f5ee09ed08473478b3ea3135d51125fbf687e402 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 18 May 2010 12:32:39 +0200 Subject: [PATCH] tests: do not ignore virInitialize failure * tests/nodeinfotest.c (mymain): Do not ignore virInitialize failure. Most other callers of virInitialize test for failure. --- tests/nodeinfotest.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index ff056b9..d3c500d 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -106,7 +106,8 @@ mymain(int argc, char **argv) return(EXIT_FAILURE); } - virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE; for (i = 0 ; i < ARRAY_CARDINALITY(nodeData); i++) if (virtTestRun(nodeData[i], 1, linuxTestNodeInfo, nodeData[i]) != 0) -- 1.7.1.250.g7d1e8

3 5

[libvirt] [PATCH] ebtablesAddRemoveRule, iptablesAddRemoveRule: don't skip va_end
by Jim Meyering 18 May '10

18 May '10

Coverity spotted two missing uses of va_end: >From 4baa4228a74f640fe789914bd034e4c5b805cdfb Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 18 May 2010 14:42:33 +0200 Subject: [PATCH] ebtablesAddRemoveRule, iptablesAddRemoveRule: don't skip va_end * src/util/ebtables.c (ebtablesAddRemoveRule): Don't skip va_end(args) on an error path. * src/util/iptables.c (iptablesAddRemoveRule): Identical change. --- src/util/ebtables.c | 7 +++++-- src/util/iptables.c | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/util/ebtables.c b/src/util/ebtables.c index e2b9608..f707756 100644 --- a/src/util/ebtables.c +++ b/src/util/ebtables.c @@ -207,15 +207,18 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) if (!(argv[n++] = strdup(arg))) goto error; va_start(args, arg); - while ((s = va_arg(args, const char *))) - if (!(argv[n++] = strdup(s))) + while ((s = va_arg(args, const char *))) { + if (!(argv[n++] = strdup(s))) { + va_end(args); goto error; + } + } va_end(args); if (!(rule = virArgvToString(&argv[command_idx]))) goto error; diff --git a/src/util/iptables.c b/src/util/iptables.c index 4f95a02..d06b857 100644 --- a/src/util/iptables.c +++ b/src/util/iptables.c @@ -134,15 +134,18 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) if (!(argv[n++] = strdup(arg))) goto error; va_start(args, arg); - while ((s = va_arg(args, const char *))) - if (!(argv[n++] = strdup(s))) + while ((s = va_arg(args, const char *))) { + if (!(argv[n++] = strdup(s))) { + va_end(args); goto error; + } + } va_end(args); if (virRun(argv, NULL) < 0) { retval = errno; goto error; -- 1.7.1.250.g7d1e8

2 1

[libvirt] [PATCH] qemudDomainRestore: handle a case of virDomainSaveStatus failure
by Jim Meyering 18 May '10

18 May '10

For each of the other uses of virDomainSaveStatus, the caller handles failure. Here it was ignored. At first I was inclined to simply "goto endjob;", like a few other uses in this file, but probing a bit, I found a few paths by which it could fail with no diagnostic whatsoever. And even if we've already emitted a diagnostic about some lower-level failure, it probably helps the user to know what high-level operation failed. >From af983f0c730fcecf90a8c948a7537095bc2e80e7 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 18 May 2010 14:07:18 +0200 Subject: [PATCH] qemudDomainRestore: handle a case of virDomainSaveStatus failure * src/qemu/qemu_driver.c (qemudDomainRestore): Don't ignore virDomainSaveStatus failure. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 948ab5b..5649a20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6167,25 +6167,28 @@ static int qemudDomainRestore(virConnectPtr conn, if (header.was_running) { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuMonitorStartCPUs(priv->mon, conn) < 0) { if (virGetLastError() == NULL) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to resume domain")); qemuDomainObjExitMonitorWithDriver(driver,vm); goto endjob; } qemuDomainObjExitMonitorWithDriver(driver, vm); vm->state = VIR_DOMAIN_RUNNING; - virDomainSaveStatus(driver->caps, driver->stateDir, vm); + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) { + VIR_WARN("Failed to save status on vm %s", vm->def->name); + goto endjob; + } } ret = 0; endjob: if (vm && qemuDomainObjEndJob(vm) == 0) vm = NULL; cleanup: virDomainDefFree(def); VIR_FREE(xml); if (fd != -1) -- 1.7.1.250.g7d1e8

2 1

[libvirt] python module set-up ignores virInitialize failure
by Jim Meyering 18 May '10

18 May '10

I've just fixed code in a test that ignored virInitialize failure. Looking at all uses, I saw one other: in python/libvirt-override.c, where the initialization function ignores virInitialize failure: void #ifndef __CYGWIN__ initlibvirtmod #else initcygvirtmod #endif (void) { static int initialized = 0; if (initialized != 0) return; virInitialize(); /* initialize the python extension module */ Py_InitModule((char *) #ifndef __CYGWIN__ "libvirtmod" #else "cygvirtmod" #endif , libvirtMethods); initialized = 1; } Unfortunately, this function is public, so we can't change its signature. Any suggestions? For reference, here's the function definition. It shows that there are many ways in which virInitialize can fail, including its many registration functions: /** * virInitialize: * * Initialize the library. It's better to call this routine at startup * in multithreaded applications to avoid potential race when initializing * the library. * * Returns 0 in case of success, -1 in case of error */ int virInitialize(void) { if (initialized) return(0); initialized = 1; if (virThreadInitialize() < 0 || virErrorInitialize() < 0 || virRandomInitialize(time(NULL) ^ getpid())) return -1; gcry_control(GCRYCTL_SET_THREAD_CBS, &virTLSThreadImpl); gcry_check_version(NULL); virLogSetFromEnv(); DEBUG0("register drivers"); #if HAVE_WINSOCK2_H if (winsock_init () == -1) return -1; #endif if (!bindtextdomain(GETTEXT_PACKAGE, LOCALEBASEDIR)) return (-1); /* * Note that the order is important: the first ones have a higher * priority when calling virConnectOpen. */ #ifdef WITH_DRIVER_MODULES /* We don't care if any of these fail, because the whole point * is to allow users to only install modules they want to use. * If they try to open a connection for a module that * is not loaded they'll get a suitable error at that point */ virDriverLoadModule("test"); virDriverLoadModule("xen"); virDriverLoadModule("openvz"); virDriverLoadModule("vbox"); virDriverLoadModule("esx"); virDriverLoadModule("xenapi"); virDriverLoadModule("remote"); #else # ifdef WITH_TEST if (testRegister() == -1) return -1; # endif # ifdef WITH_XEN if (xenRegister () == -1) return -1; # endif # ifdef WITH_OPENVZ if (openvzRegister() == -1) return -1; # endif # ifdef WITH_PHYP if (phypRegister() == -1) return -1; # endif # ifdef WITH_VBOX if (vboxRegister() == -1) return -1; # endif # ifdef WITH_ESX if (esxRegister() == -1) return -1; # endif # ifdef WITH_XENAPI if (xenapiRegister() == -1) return -1; # endif # ifdef WITH_REMOTE if (remoteRegister () == -1) return -1; # endif #endif return(0); }

3 6

[libvirt] [PATCH] qemuMonitorTextMigrate: avoid leak on OOM-error path
by Jim Meyering 18 May '10

18 May '10

Oops. Nearly forgot to post this one: >From f1b58f1f7bd287b4a2157f62ebb63100b45a8b62 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 18 May 2010 12:17:23 +0200 Subject: [PATCH] qemuMonitorTextMigrate: avoid leak on OOM-error path * src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrate): Also free "safedest" buffer when failing. --- src/qemu/qemu_monitor_text.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index ae5d4d2..ec3d69d 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1149,6 +1149,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, if (virBufferError(&extra)) { virBufferFreeAndReset(&extra); virReportOOMError(); + free(safedest); return -1; } if (virAsprintf(&cmd, "migrate %s\"%s\"", virBufferContentAndReset(&extra), safedest) < 0) { -- 1.7.1.250.g7d1e8

2 1

[libvirt] handling qemuMonitorAddDevice failure: missing drive_del function?
by Jim Meyering 18 May '10

18 May '10

In src/qemu/qemu_driver.c, coverity gripes (rightly) about this: 6912 qemuDomainObjEnterMonitorWithDriver(driver, vm); 6913 if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { 6914 ret = qemuMonitorAddDrive(priv->mon, drivestr); 6915 if (ret == 0) No check of the return value of "qemuMonitorAddDevice(priv->mon, devstr)". Calling function "qemuMonitorAddDevice" without checking return value. 6916 qemuMonitorAddDevice(priv->mon, devstr); 6917 /* XXX remove the drive upon fail */ 6918 } else { Does anyone have a preference on how to deal with it while we wait for a drive-removal function? I think it deserves at least a diagnostic. I suppose this comment is still relevant: if (ret == 0) ret = qemuMonitorAddDevice(priv->mon, devstr); /* XXX should call 'drive_del' on error but this does not exist yet */ This XXX marks the same problem -- though note that coverity could not possibly see this one.

2 3

[libvirt] [PATCH] Init script for suspending/resuming guests on shutdown/boot
by Jiri Denemark 18 May '10

18 May '10

Example output during shutdown: Running guests on default URI: console, rhel6-1, rhel5-64 Running guests on lxc:/// URI: lxc-shell Running guests on xen:/// URI: error: no hypervisor driver available for xen:/// error: failed to connect to the hypervisor Running guests on vbox+tcp://orkuz/system URI: no running guests. Suspending guests on default URI... Suspending console: done Suspending rhel6-1: done Suspending rhel5-64: done Suspending guests on lxc:/// URI... Suspending lxc-shell: error: Failed to save domain 9cba8bfb-56f4-6589-2d12-8a58c886dd3b state error: this function is not supported by the hypervisor: virDomainManagedSave Note, the "Suspending $guest: " shows progress during the suspend phase if domjobinfo gives meaningful output. Example output during boot: Resuming guests on default URI... Resuming guest rhel6-1: done Resuming guest rhel5-64: done Resuming guest console: done Resuming guests on lxc:/// URI... Resuming guest lxc-shell: already active Configuration used for generating the examples above: URIS='default lxc:/// xen:/// vbox+tcp://orkuz/system' The script uses /var/lib/libvirt/libvirt-guests files to note all active guest it should try to resume on next boot. It's content looks like: default 7f8b9d93-30e1-f0b9-47a7-cb408482654b 085b4c95-5da2-e8e1-712f-6ea6a4156af2 fb4d8360-5305-df3a-2da1-07d682891b8c lxc:/// 9cba8bfb-56f4-6589-2d12-8a58c886dd3b --- daemon/Makefile.am | 16 +++- daemon/libvirt-guests.init.in | 194 +++++++++++++++++++++++++++++++++++++++++ daemon/libvirt-guests.sysconf | 3 + 3 files changed, 209 insertions(+), 4 deletions(-) create mode 100644 daemon/libvirt-guests.init.in create mode 100644 daemon/libvirt-guests.sysconf diff --git a/daemon/Makefile.am b/daemon/Makefile.am index a82e9a9..ed469bf 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -29,6 +29,8 @@ EXTRA_DIST = \ libvirtd.lxc.logrotate.in \ libvirtd.uml.logrotate.in \ test_libvirtd.aug \ + libvirt-guests.init.in \ + libvirt-guests.sysconf \ $(AVAHI_SOURCES) \ $(DAEMON_SOURCES) @@ -216,21 +218,27 @@ install-logrotate: $(LOGROTATE_CONFS) $(INSTALL_DATA) libvirtd.uml.logrotate $(DESTDIR)$(sysconfdir)/logrotate.d/libvirtd.uml if LIBVIRT_INIT_SCRIPT_RED_HAT -install-init: libvirtd.init +install-init: libvirtd.init libvirt-guests.init mkdir -p $(DESTDIR)$(sysconfdir)/rc.d/init.d $(INSTALL_SCRIPT) libvirtd.init \ $(DESTDIR)$(sysconfdir)/rc.d/init.d/libvirtd + $(INSTALL_SCRIPT) libvirt-guests.init \ + $(DESTDIR)$(sysconfdir)/rc.d/init.d/libvirt-guests mkdir -p $(DESTDIR)$(sysconfdir)/sysconfig $(INSTALL_SCRIPT) $(srcdir)/libvirtd.sysconf \ $(DESTDIR)$(sysconfdir)/sysconfig/libvirtd + $(INSTALL_SCRIPT) $(srcdir)/libvirt-guests.sysconf \ + $(DESTDIR)$(sysconfdir)/sysconfig/libvirt-guests uninstall-init: rm -f $(DESTDIR)$(sysconfdir)/rc.d/init.d/libvirtd \ - $(DESTDIR)$(sysconfdir)/sysconfig/libvirtd + $(DESTDIR)$(sysconfdir)/sysconfig/libvirtd \ + $(DESTDIR)$(sysconfdir)/rc.d/init.d/libvirt-guests \ + $(DESTDIR)$(sysconfdir)/sysconfig/libvirt-guests -BUILT_SOURCES += libvirtd.init +BUILT_SOURCES += libvirtd.init libvirt-guests.init -libvirtd.init: libvirtd.init.in +%.init: %.init.in $(AM_V_GEN)sed \ -e s!\@localstatedir\@!@localstatedir@!g \ -e s!\@sbindir\@!@sbindir@!g \ diff --git a/daemon/libvirt-guests.init.in b/daemon/libvirt-guests.init.in new file mode 100644 index 0000000..e930115 --- /dev/null +++ b/daemon/libvirt-guests.init.in @@ -0,0 +1,194 @@ +#!/bin/sh + +# the following is the LSB init header see +# http://www.linux-foundation.org/spec//booksets/LSB-Core-generic/LSB-Core-ge… +# +### BEGIN INIT INFO +# Provides: libvirt-guests +# Required-Start: libvirtd +# Required-Stop: libvirtd +# Default-Start: 3 4 5 +# Short-Description: suspend/resume libvirt guests on shutdown/boot +# Description: This is a script for suspending active libvirt guests +# on shutdown and resuming them on next boot +# See http://libvirt.org +### END INIT INFO + +# the following is chkconfig init header +# +# libvirt-guests: suspend/resume libvirt guests on shutdown/boot +# +# chkconfig: 345 98 02 +# description: This is a script for suspending active libvirt guests +# on shutdown and resuming them on next boot +# See http://libvirt.org +# + +sysconfdir=@sysconfdir@ +localstatedir=@localstatedir@ + +# Source function library. +. $sysconfdir/rc.d/init.d/functions + +URIS=default + +test -f $sysconfdir/sysconfig/libvirt-guests && . $sysconfdir/sysconfig/libvirt-guests + +LISTFILE=$localstatedir/lib/libvirt/libvirt-guests + +RETVAL=0 + +retval() { + "$@" + if [ $? -ne 0 ]; then + RETVAL=1 + return 1 + else + return 0 + fi +} + +run_virsh() { + uri=$1 + shift + + if [ "x$uri" = xdefault ]; then + conn= + else + conn="-c $uri" + fi + + virsh $conn "$@" +} + +run_virsh_c() { + ( export LC_ALL=C; run_virsh "$@" ) +} + +list_guests() { + uri=$1 + + list=`run_virsh_c $uri list` + if [ $? -ne 0 ]; then + RETVAL=1 + return 1 + fi + + for id in `echo "$list" | awk 'NR > 2 {print $1}'`; do + run_virsh_c $uri dominfo $id | awk '/^UUID:/{print $2}' + done +} + +guest_name() { + uri=$1 + uuid=$2 + + name=`run_virsh_c $uri dominfo $uuid 2>/dev/null | \ + awk '/^Name:/{print $2}'` + [ -n "$name" ] || name=$uuid + + echo "$name" +} + +guest_is_on() { + uri=$1 + uuid=$2 + + id=`run_virsh_c $uri dominfo $uuid 2>/dev/null | \ + awk '/^Id:/{print $2}'` + + [ -n "$id" ] && ! [ "x$id" = x- ] +} + +start() { + while read uri list; do + configured=false + for confuri in $URIS; do + if [ $confuri = $uri ]; then + configured=true + break + fi + done + if ! $configured; then + echo $"Ignoring guests on $uri URI" + continue + fi + + echo $"Resuming guests on $uri URI..." + for guest in $list; do + name=`guest_name $uri $guest` + echo -n $"Resuming guest $name: " + if retval guest_is_on $uri $guest; then + echo $"already active" + else + retval run_virsh $uri start "$name" >/dev/null && echo $"done" + fi + done + done <$LISTFILE + + rm -f $LISTFILE +} + +stop() { + >$LISTFILE + for uri in $URIS; do + echo -n $"Running guests on $uri URI: " + list=`list_guests $uri` + if [ $? -eq 0 ]; then + empty=true + for uuid in $list; do + $empty || echo -n ", " + echo -n `guest_name $uri $uuid` + empty=false + done + if $empty; then + echo $"no running guests." + else + echo + echo $uri $list >>$LISTFILE + fi + fi + done + + while read uri list; do + echo $"Suspending guests on $uri URI..." + for guest in $list; do + name=`guest_name $uri $guest` + label=$"Suspending $name: " + echo -n "$label" + run_virsh $uri managedsave $guest >/dev/null & + virsh_pid=$! + while true; do + sleep .5 + kill -0 $virsh_pid >&/dev/null || break + progress=`run_virsh_c $uri domjobinfo $guest 2>/dev/null | \ + awk '/^Data processed:/{print $3, $4}'` + if [ -n "$progress" ]; then + printf '\r%s%12s ' "$label" "$progress" + else + printf '\r%s%-12s ' "$label" "..." + fi + done + retval wait $virsh_pid && printf '\r%s%-12s\n' "$label" $"done" + done + done <$LISTFILE +} + +gueststatus() { + for uri in $URIS; do + echo "* $uri URI:" + retval run_virsh $uri list || echo + done +} + +# See how we were called. +case "$1" in + start|stop|gueststatus) + $1 + ;; + *) + echo $"Usage: $0 {start|stop|gueststatus}" + exit 3 + ;; +esac +exit $RETVAL diff --git a/daemon/libvirt-guests.sysconf b/daemon/libvirt-guests.sysconf new file mode 100644 index 0000000..eb70ebf --- /dev/null +++ b/daemon/libvirt-guests.sysconf @@ -0,0 +1,3 @@ +# specify URIs to check for running guests +# example: URIS='default xen:/// vbox+tcp://host/system lxc:///' +#URIS=default -- 1.7.1

3 5

[libvirt] [PATCH] qemuMonitorTextMigrate: avoid leak on OOM-error path
by Jim Meyering 18 May '10

18 May '10

Fix a minor leak: >From 0fc88bfcae06346e871d2a4a89febb160bc0857e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 18 May 2010 12:17:23 +0200 Subject: [PATCH] qemuMonitorTextMigrate: avoid leak on OOM-error path * src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrate): Also free "safedest" buffer when failing. --- src/qemu/qemu_monitor_text.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index ae5d4d2..ec3d69d 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1149,6 +1149,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, if (virBufferError(&extra)) { virBufferFreeAndReset(&extra); virReportOOMError(); + free(safedest); return -1; } if (virAsprintf(&cmd, "migrate %s\"%s\"", virBufferContentAndReset(&extra), safedest) < 0) { -- 1.7.1.250.g7d1e8

2 1