- Devel - libvirt List Archives

[libvirt] review fallout: short memset
by Jim Meyering 02 Dec '08

02 Dec '08

While reviewing unrelated changes, I spotted a short memset: char **names; ... memset(names, 0, maxnames); That zeros out 1/4 or 1/8 of the memory than it should. It should be doing this: memset(names, 0, maxnames * sizeof (*names)); I checked all memset uses and found a total of 6 uses like that. This fixes them: >From 614b0064eadc3ecef0690d5c65bb531844a3091d Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Tue, 2 Dec 2008 14:49:03 +0100 Subject: [PATCH] fix inadequate initialization in storage and test drivers * src/storage_driver.c (storageListPools): Set all "names" entries to 0. (storageListDefinedPools, storagePoolListVolumes): Likewise. * src/test.c (testStoragePoolListVolumes): Likewise. --- src/storage_driver.c | 8 ++++---- src/test.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/storage_driver.c b/src/storage_driver.c index 366820b..53388f1 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -347,7 +347,7 @@ storageListPools(virConnectPtr conn, free(names[i]); names[i] = NULL; } - memset(names, 0, nnames); + memset(names, 0, nnames * sizeof(*names)); return -1; } @@ -389,7 +389,7 @@ storageListDefinedPools(virConnectPtr conn, free(names[i]); names[i] = NULL; } - memset(names, 0, nnames); + memset(names, 0, nnames * sizeof(*names)); return -1; } @@ -880,7 +880,7 @@ storagePoolListVolumes(virStoragePoolPtr obj, return -1; } - memset(names, 0, maxnames); + memset(names, 0, maxnames * sizeof(*names)); for (i = 0 ; i < pool->volumes.count && n < maxnames ; i++) { if ((names[n++] = strdup(pool->volumes.objs[i]->name)) == NULL) { virStorageReportError(obj->conn, VIR_ERR_NO_MEMORY, @@ -895,7 +895,7 @@ storagePoolListVolumes(virStoragePoolPtr obj, for (n = 0 ; n < maxnames ; n++) VIR_FREE(names[i]); - memset(names, 0, maxnames); + memset(names, 0, maxnames * sizeof(*names)); return -1; } diff --git a/src/test.c b/src/test.c index 7998886..3e942da 100644 --- a/src/test.c +++ b/src/test.c @@ -1951,7 +1951,7 @@ testStoragePoolListVolumes(virStoragePoolPtr obj, POOL_IS_ACTIVE(privpool, -1); int i = 0, n = 0; - memset(names, 0, maxnames); + memset(names, 0, maxnames * sizeof(*names)); for (i = 0 ; i < privpool->volumes.count && n < maxnames ; i++) { if ((names[n++] = strdup(privpool->volumes.objs[i]->name)) == NULL) { testError(obj->conn, VIR_ERR_NO_MEMORY, "%s", _("name")); @@ -1965,7 +1965,7 @@ testStoragePoolListVolumes(virStoragePoolPtr obj, for (n = 0 ; n < maxnames ; n++) VIR_FREE(names[i]); - memset(names, 0, maxnames); + memset(names, 0, maxnames * sizeof(*names)); return -1; } -- 1.6.0.4.1044.g77718

3 2

[libvirt] [PATCH 2/2]: Call udevsettle in the appropriate places
by Chris Lalancette 02 Dec '08

02 Dec '08

Instead of relying solely on polling for /dev devices to appear in libvirt, we really should be synchronizing against udev. This is generally done by a call to udevsettle, which is exactly what this patch implements for the storage backends that are likely to create new /dev nodes. I believe I've read that even after udevsettle, you are not guaranteed that devices are all the way created, so we still need the polling in the rest of the sources, but this should give us a much better chance of things existing as we expect. Signed-off-by: Chris Lalancette <clalance(a)redhat.com> Index: src/storage_backend.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.c,v retrieving revision 1.29 diff -u -r1.29 storage_backend.c --- a/src/storage_backend.c 17 Nov 2008 11:19:33 -0000 1.29 +++ b/src/storage_backend.c 26 Nov 2008 12:40:19 -0000 @@ -270,6 +270,20 @@ return 0; } +void virStorageBackendWaitForDevices(virConnectPtr conn) +{ + const char *const settleprog[] = { UDEVSETTLE, NULL }; + int exitstatus; + + /* + * NOTE: we ignore errors here; this is just to make sure that any device + * nodes that are being created finish before we try to + * scan them. If this fails (udevsettle exits badly, or doesn't exist + * at all), then we still have a fallback mechanism anyway. + */ + virRun(conn, settleprog, &exitstatus); +} + /* * Given a volume path directly in /dev/XXX, iterate over the * entries in the directory pool->def->target.path and find the Index: src/storage_backend.h =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.h,v retrieving revision 1.13 diff -u -r1.13 storage_backend.h --- a/src/storage_backend.h 17 Nov 2008 11:19:33 -0000 1.13 +++ b/src/storage_backend.h 26 Nov 2008 12:40:19 -0000 @@ -69,6 +69,8 @@ int fd, int withCapacity); +void virStorageBackendWaitForDevices(virConnectPtr conn); + char *virStorageBackendStablePath(virConnectPtr conn, virStoragePoolObjPtr pool, const char *devpath); Index: src/storage_backend_logical.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend_logical.c,v retrieving revision 1.26 diff -u -r1.26 storage_backend_logical.c --- a/src/storage_backend_logical.c 17 Nov 2008 11:19:33 -0000 1.26 +++ b/src/storage_backend_logical.c 26 Nov 2008 12:40:19 -0000 @@ -470,6 +470,8 @@ }; int exitstatus; + virStorageBackendWaitForDevices(conn); + /* Get list of all logical volumes */ if (virStorageBackendLogicalFindLVs(conn, pool, NULL) < 0) { virStoragePoolObjClearVols(pool); Index: src/storage_backend_disk.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend_disk.c,v retrieving revision 1.20 diff -u -r1.20 storage_backend_disk.c --- a/src/storage_backend_disk.c 17 Nov 2008 11:19:33 -0000 1.20 +++ b/src/storage_backend_disk.c 26 Nov 2008 12:40:19 -0000 @@ -262,6 +262,8 @@ VIR_FREE(pool->def->source.devices[0].freeExtents); pool->def->source.devices[0].nfreeExtent = 0; + virStorageBackendWaitForDevices(conn); + return virStorageBackendDiskReadPartitions(conn, pool, NULL); } Index: src/storage_backend_iscsi.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend_iscsi.c,v retrieving revision 1.18 diff -u -r1.18 storage_backend_iscsi.c --- a/src/storage_backend_iscsi.c 17 Nov 2008 11:19:33 -0000 1.18 +++ b/src/storage_backend_iscsi.c 26 Nov 2008 12:40:19 -0000 @@ -603,6 +603,8 @@ pool->def->allocation = pool->def->capacity = pool->def->available = 0; + virStorageBackendWaitForDevices(conn); + if ((session = virStorageBackendISCSISession(conn, pool)) == NULL) goto cleanup; if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0) Index: configure.in =================================================================== RCS file: /data/cvs/libvirt/configure.in,v retrieving revision 1.187 diff -u -r1.187 configure.in --- a/configure.in 25 Nov 2008 15:48:11 -0000 1.187 +++ b/configure.in 26 Nov 2008 12:40:19 -0000 @@ -115,11 +115,15 @@ [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([BRCTL], [brctl], [brctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([UDEVSETTLE], [udevsettle], [udevsettle], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) AC_DEFINE_UNQUOTED([BRCTL],["$BRCTL"], [Location or name of the brctl program (see bridge-utils)]) +AC_DEFINE_UNQUOTED([UDEVSETTLE],["$UDEVSETTLE"], + [Location or name of the udevsettle program (see udev)]) dnl Specific dir for HTML output ? AC_ARG_WITH([html-dir], [AC_HELP_STRING([--with-html-dir=path],

5 13

[libvirt] Concerning umlInotifyEvent
by Ron Yorston 02 Dec '08

02 Dec '08

At the top of umlInotifyEvent in uml_driver.c there is this test: if (watch != driver->inotifyWatch) return; This doesn't seem to be correct. I have to comment out the test to get libvirtd to work with UML. Ron

2 3

[libvirt] libvirt 0.5.0 and KVM migration
by Mickaël Canévet 02 Dec '08

02 Dec '08

Hi, I just installed libvirt 0.5.0 on Debian Lenny with KVM 0.72 to try KVM migration support. When I try to migrate a VM from one node to the other one, I have this error message: libvir: error : this function is not supported by the hypervisor: virDomainMigrate Is it really supposed to work ? Do I have to upgrade KVM to a newer version ? Thank you for your answers and congratulation for your precious work. -- _______________________________________________________________________ Mickaël Canévet. European Molecular Biology Laboratory (EMBL) Grenoble Outstation. FRANCE _______________________________________________________________________

3 6

[libvirt] Doubt over Kvm migration support in libvirt 0.5.0
by Shanmuga Rajan 02 Dec '08

02 Dec '08

I am using libvirt(python) to develop a small application which will manage my nodes which running in KVM. But when i tried migration i got this error. >>> vs.migrate(conn, libvirt.VIR_MIGRATE_LIVE,'testtt','192.168.1.82',0) libvir: error : this function is not supported by the hypervisor: virDomainMigrate Traceback (most recent call last): File "<stdin>", line 1, in ? File "/usr/lib64/python2.4/site-packages/libvirt.py", line 301, in migrate if ret is None:raise libvirtError('virDomainMigrate() failed', dom=self) libvirt.libvirtError: virDomainMigrate() failed this function is not supported by the hypervisor: virDomainMigrate i am running kvm-66 taken from Centos-5 Testing repositary and libvirt 0.5.0( 0.5.0: Nov 25 2008 release note says that support for kvm migration included) i am bit confused after reading this... Please any one help me in this. Thanks and Regards, -Shan

3 2

[libvirt] [PATCH] qemud/qemud.c (qemudCleanup): Plug a leak.
by Jim Meyering 01 Dec '08

01 Dec '08

Hi Dan, With your patches, "make check" hits one new leak: 40 bytes in 1 blocks are definitely lost in loss record 9 of 65 at 0x4A05174: calloc (vg_replace_malloc.c:397) by 0x447F29: virAllocN (memory.c:128) by 0x40CD82: qemudRunLoop (qemud.c:1839) by 0x40E713: main (qemud.c:2462) This fixes it: >From 9230c41d3deb00b4a0bb6800126e5cd41a350a54 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 1 Dec 2008 15:07:40 +0100 Subject: [PATCH] * qemud/qemud.c (qemudCleanup): Plug a leak. --- qemud/qemud.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index 94f5918..8518bd5 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1942,6 +1942,7 @@ static void qemudCleanup(struct qemud_server *server) { virStateCleanup(); + VIR_FREE(server->workers); free(server); } -- 1.6.0.4.1044.g77718

1 0

[libvirt] avoid "make check" hang on daemon-conf
by Jim Meyering 01 Dec '08

01 Dec '08

Hi Dan, With your 28-part patch, I was getting a daemon-conf failure in "make check". This fixes it: Unless you tell me otherwise, I will apply most of this right away, (and let you remove the space-before-max_clients in your patch) since no other part of this change conflicts with your series. >From 57842865f8845c788408e18033f739cf02987ca3 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 1 Dec 2008 14:12:44 +0100 Subject: [PATCH] tests: daemon-conf: accommodate new numeric-valued config params: This test would hang when failing to perturb the new numeric (and non-boolean) valued parameters, max_clients, max_workers, min_workers. * tests/daemon-conf: Require that all commented out settings in libvirtd.conf have the same form. Before, two parameters were not being tested, since a space had snuck between the leading "#" and the "param = value" parts. Apply each RHS-value-perturbing transformation separately, not in series. Let VERBOSE=yes turn on debugging. Be more verbose by default, since this is a relatively long-running test. * qemud/libvirtd.conf: Normalize the spacing around each setting that is to be perturbed by tests/daemon-conf. --- qemud/libvirtd.conf | 10 ++++++---- tests/daemon-conf | 32 ++++++++++++++++++++++++++------ 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/qemud/libvirtd.conf b/qemud/libvirtd.conf index f269766..b0962f2 100644 --- a/qemud/libvirtd.conf +++ b/qemud/libvirtd.conf @@ -1,7 +1,10 @@ # Master libvirt daemon configuration file # # For further information consult http://libvirt.org/format.html - +# +# NOTE: the tests/daemon-conf regression test script requires +# that each "PARAMETER = VALUE" line in this file have the parameter +# name just after a leading "#". ################################################################# # @@ -45,7 +48,7 @@ # Override the default configuration which binds to all network # interfaces. This can be a numeric IPv4/6 address, or hostname # -# listen_addr = "192.168.0.1" +#listen_addr = "192.168.0.1" # Flag toggling mDNS advertizement of the libvirt service. @@ -233,7 +236,7 @@ # The maximum number of concurrent client connections to allow # over all sockets combined. -# max_clients = 20 +#max_clients = 20 # The minimum limit sets the number of workers to start up @@ -243,4 +246,3 @@ # of clients allowed #min_workers = 5 #max_workers = 20 - diff --git a/tests/daemon-conf b/tests/daemon-conf index db1f0d3..03189d5 100755 --- a/tests/daemon-conf +++ b/tests/daemon-conf @@ -1,6 +1,11 @@ #!/bin/sh # Get coverage of libvirtd's config-parsing code. +if test "$VERBOSE" = yes; then + set -x + libvirtd --version +fi + # Boilerplate code to set up a test directory, cd into it, # and to ensure we remove it upon completion. this_test_() { echo "./$0" | sed 's,.*/,,'; } @@ -12,8 +17,14 @@ trap '(exit $?); exit $?' 1 2 13 15 mkdir "$t_" || fail=1 cd "$t_" || fail=1 +conf="$abs_top_srcdir/qemud/libvirtd.conf" + +# Ensure that each commented out PARAMETER = VALUE line has the expected form. +grep '[a-z_] *= *[^ ]' "$conf" | grep -vE '^#[a-z_]+ = ' \ + && { echo "$0: found unexpected lines (above) in $conf" 1>&2; exit 1; } + # Start with the sample libvirtd.conf file, uncommenting all real directives. -sed -n 's/^#$[^ #]$/\1/p' $abs_top_srcdir/qemud/libvirtd.conf > tmp.conf +sed -n 's/^#$[^ #]$/\1/p' "$conf" > tmp.conf # Iterate through that list of directives, corrupting one RHS at a # time and running libvirtd with the resulting config. Each libvirtd @@ -22,16 +33,23 @@ n=$(wc -l < tmp.conf) i=1 while :; do param_name=$(sed -n "$i"'s/ = .*//p' tmp.conf) + printf "testing with corrupted config: $param_name\n" 1>&2 rhs=$(sed -n "$i"'s/.* = $.*$/\1/p' tmp.conf) f=in$i.conf - # Change an RHS that starts with '"' or '[' to "3". - # Change an RHS that starts with 0 or 1 to the string '"foo"'. - sed "$i"'s/ = [["].*/ = 3/;'"$i"'s/ = [01].*/ = "foo"/' tmp.conf > $f + case $rhs in + # Change an RHS that starts with '"' or '[' to "3". + [[\"]*) sed "$i"'s/ = [["].*/ = 3/' tmp.conf > $f;; + # Change an RHS that starts with a digit to the string '"foo"'. + [0-9]*) sed "$i"'s/ = [0-9].*/ = "foo"/' tmp.conf > $f;; + esac + + # Run libvirtd, expecting it to fail. libvirtd --config=$f 2> err && fail=1 + case $rhs in # '"'*) msg='should be a string';; '"'*) msg='invalid type: got long; expected string';; - [01]*) msg='invalid type: got string; expected long';; + [0-9]*) msg='invalid type: got string; expected long';; '['*) msg='must be a string or list of strings';; *) echo "unexpected RHS: $rhs" 1>&2; fail=1;; esac @@ -48,8 +66,10 @@ while :; do done # Run with the unmodified config file. +sleep_secs=2 +printf "running libvirtd with a valid config file ($sleep_secs seconds)\n" 1>&2 libvirtd --config=tmp.conf > log 2>&1 & pid=$! -sleep 2 +sleep $sleep_secs kill $pid # Expect an orderly shut-down and successful exit. -- 1.6.0.4.1044.g77718

1 0

[libvirt] KVM/qemu: problems with autostart of vms with non-bridged nets
by Gerd von Egidy 01 Dec '08

01 Dec '08

Hi, I'm using libvirt 0.5.0 to manage my kvm domains. Everything works fine except autostarting vms which use a network device not directly bridged to hw. I have the following net defined (and set to autostart): <network> <name>local</name> <uuid>7ec7a457-0b8e-4670-3b4a-2d0914288daa</uuid> <bridge stp='on' forwardDelay='0' /> <ip address='192.168.100.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.100.128' end='192.168.100.254' /> </dhcp> </ip> </network> When a vm using this net is autostarted I get the following error: libvir: error : invalid connection pointer in virNetworkLookupByName libvir: QEMU error : internal error Network 'local' not found Failed to autostart VM 'testlocalnet': internal error Network 'local' not found I took a dive into the code and found two problems causing this: 1. initialization order: qemuStateDriver is registered (and thus initialized) before networkStateDriver. This means the network is not up yet when qemu does it's autostart. I could fix this with the attached patch. 2. missing virConnectPtr during autostart of the vm: in src/qemu_driver.c:145, qemudAutostartConfigs() you can find this call: int ret = qemudStartVMDaemon(NULL, driver, vm, NULL); This means virConnectPtr conn is NULL within qemudStartVMDaemon(). Following the calls: qemudStartVMDaemon() -> qemudBuildCommandLine() -> qemudNetworkIfaceConnect() -> virNetworkLookupByName() virNetworkLookupByName bails out if called with invalid virConnectPtr. This means starting this vm fails. Currently I'm not familiar enough with the libvirt structures to come up with an idea how to fix this. Could anybody with a deeper knowledge of the code take a look at this? Thank you very much. Kind regards, Gerd diff -r -u libvirt-0.5.0.orig/qemud/qemud.c libvirt-0.5.0/qemud/qemud.c --- libvirt-0.5.0.orig/qemud/qemud.c 2008-11-21 13:47:32.000000000 +0100 +++ libvirt-0.5.0/qemud/qemud.c 2008-11-30 21:27:06.000000000 +0100 @@ -761,22 +761,13 @@ * If they try to use a open a connection for a module that * is not loaded they'll get a suitable error at that point */ - virDriverLoadModule("qemu"); - virDriverLoadModule("lxc"); - virDriverLoadModule("uml"); virDriverLoadModule("network"); virDriverLoadModule("storage"); virDriverLoadModule("nodedev"); + virDriverLoadModule("qemu"); + virDriverLoadModule("lxc"); + virDriverLoadModule("uml"); #else -#ifdef WITH_QEMU - qemuRegister(); -#endif -#ifdef WITH_LXC - lxcRegister(); -#endif -#ifdef WITH_UML - umlRegister(); -#endif #ifdef WITH_NETWORK networkRegister(); #endif @@ -786,6 +777,15 @@ #if defined(HAVE_HAL) || defined(HAVE_DEVKIT) nodedevRegister(); #endif +#ifdef WITH_QEMU + qemuRegister(); +#endif +#ifdef WITH_LXC + lxcRegister(); +#endif +#ifdef WITH_UML + umlRegister(); +#endif #endif virEventRegisterImpl(virEventAddHandleImpl, -- Address (better: trap) for people I really don't want to get mail from: james(a)cactusamerica.com

2 2

[libvirt] [PATCH] avoid format string warnings
by Jim Meyering 01 Dec '08

01 Dec '08

Hi Dan, I've just applied all of your changes, built, and saw some new warnings: uml_driver.c:1742: warning: format not a string literal and no format arguments uml_driver.c:1748: warning: format not a string literal and no format arguments uml_driver.c:1785: warning: format not a string literal and no format arguments openvz_conf.c:468: warning: format not a string literal and no format arguments But they were introduced before your changes. Here's the patch, on top of your changes. Note that as is, the patch does not apply to the latest in CVS, so my checking it in there would induce merge conflicts for you. So I'll hold off until after your 28-part series is committed. Jim >From fb32deb5dfb2a3c6b598de619f25c0bae4314a05 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 1 Dec 2008 11:17:08 +0100 Subject: [PATCH] avoid format string warnings * src/uml_driver.c (umlDomainBlockPeek): Put "%s" before each literal format string with no % directive. * src/openvz_conf.c (openvzGetNodeCPUs): Likewise. --- src/openvz_conf.c | 2 +- src/uml_driver.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/openvz_conf.c b/src/openvz_conf.c index fc4ef20..1361c53 100644 --- a/src/openvz_conf.c +++ b/src/openvz_conf.c @@ -466,7 +466,7 @@ openvzGetNodeCPUs(void) if (virNodeInfoPopulate(NULL, &nodeinfo) < 0) { openvzError(NULL, VIR_ERR_INTERNAL_ERROR, - _("Cound not read nodeinfo")); + "%s", _("Cound not read nodeinfo")); return 0; } diff --git a/src/uml_driver.c b/src/uml_driver.c index d2f2c7b..8d33c70 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -1740,13 +1740,13 @@ umlDomainBlockPeek (virDomainPtr dom, if (!vm) { umlReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, - _("no domain with matching uuid")); + "%s", _("no domain with matching uuid")); goto cleanup; } if (!path || path[0] == '\0') { umlReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, - _("NULL or empty path")); + "%s", _("NULL or empty path")); goto cleanup; } @@ -1783,7 +1783,7 @@ umlDomainBlockPeek (virDomainPtr dom, ret = 0; } else { umlReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, - _("invalid path")); + "%s", _("invalid path")); } cleanup: -- 1.6.0.4.1044.g77718

1 0

[libvirt] Memory leak in xen_inotify.c
by Niraj Tolia 30 Nov '08

30 Nov '08

Hi, It looks like xenInotifyOpen() in xen_inotify.c has an opendir() call. However, as there is no corresponding closedir() and opendir() uses malloc internally, valgrind reports this as a memory leak. Adding a closedir() to the end of this function should fix this. Cheers, Niraj

2 1