- Devel - libvirt List Archives

[libvirt] [PATCH] virStorageBackendIsMultipath: avoid dead store
by Jim Meyering 11 Feb '10

11 Feb '10

This suppresses a warning reported by clang. Since "target_type" is initialized, there's no point in storing dm_get_next_target's return value. >From d0cd9b4233194b27fb168e835223dd51e77fa6f2 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Thu, 11 Feb 2010 15:26:37 +0100 Subject: [PATCH] virStorageBackendIsMultipath: avoid dead store * src/storage/storage_backend_mpath.c (virStorageBackendIsMultipath): The result of dm_get_next_target was never used (and isn't needed), so don't store it. --- src/storage/storage_backend_mpath.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index dbcd6d8..1e7504b 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -1,7 +1,7 @@ /* * storage_backend_mpath.c: storage backend for multipath handling * - * Copyright (C) 2009-2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * Copyright (C) 2009-2008 Dave Allan * * This library is free software; you can redistribute it and/or @@ -150,7 +150,6 @@ virStorageBackendIsMultipath(const char *devname) { int ret = 0; struct dm_task *dmt = NULL; - void *next = NULL; uint64_t start, length; char *target_type = NULL; char *params = NULL; @@ -173,8 +172,7 @@ virStorageBackendIsMultipath(const char *devname) goto out; } - next = dm_get_next_target(dmt, next, &start, &length, - &target_type, &params); + dm_get_next_target(dmt, next, &start, &length, &target_type, &params); if (target_type == NULL) { ret = -1; -- 1.7.0.rc2.170.gbc565

2 1

[libvirt] [PATCH] Add virConnectGetVersion Python API
by Taizo ITO 11 Feb '10

11 Feb '10

Hi all, The attached patch adds a new python API call for retrieving the running hypervisor version used by a connection: virConnectGetVersion. Regards, Taizo -- Taizo ITO

2 1

[libvirt] [PATCH 4/5] macvtap support for libvirt -- helper code
by Stefan Berger 11 Feb '10

11 Feb '10

This part adds the helper code to setup and tear down macvtap devices using direct communication with the device driver via netlink sockets. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>

2 3

[libvirt] [PATCH 3/5] macvtap support for libvirt -- qemu support
by Stefan Berger 11 Feb '10

11 Feb '10

This part adds support for qemu making a macvtap tap device available via file descriptor passed to qemu command line. This also attempts to tear down the macvtap device when a VM terminates. This includes support for attachment and detachment to/from running VM. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>

3 4

[libvirt] [PATCH 2/5] macvtap support for libvirt -- parse new interface XML
by Stefan Berger 11 Feb '10

11 Feb '10

This part adds support to domain_conf.{c|h} for parsing the new interface XML of type 'direct'. The parsed mode is now stored as an int. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>

3 3

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

11 Feb '10

Hello! This is a re-post of previously posted patches following Daniel Berrange's request for changes along with other fixes. 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 3

[libvirt] [PATCH 5/5] macvtap support for libvirt -- schema extensions
by Stefan Berger 11 Feb '10

11 Feb '10

This part extends the domain xml schema to support the new interface type 'direct'. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>

2 1

[libvirt] virStoragePoolSourceFree(S) does not free S
by Jim Meyering 11 Feb '10

11 Feb '10

I was surprised to see that virStoragePoolSourceFree(S) does not free S. The other three vir*Free functions in storage_conf *do* free S. There are at least three places where this causes leaks. Here's one: virStoragePoolSourcePtr virStoragePoolDefParseSourceString(virConnectPtr conn, const char *srcSpec, int pool_type) { xmlDocPtr doc = NULL; xmlNodePtr node = NULL; xmlXPathContextPtr xpath_ctxt = NULL; virStoragePoolSourcePtr def = NULL, ret = NULL; doc = xmlReadDoc((const xmlChar *)srcSpec, "srcSpec.xml", NULL, XML_PARSE_NOENT | XML_PARSE_NONET | XML_PARSE_NOERROR | XML_PARSE_NOWARNING); if (doc == NULL) { virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s", _("bad <source> spec")); goto cleanup; } xpath_ctxt = xmlXPathNewContext(doc); if (xpath_ctxt == NULL) { virReportOOMError(conn); goto cleanup; } if (VIR_ALLOC(def) < 0) { <<<====== def is alloc'd virReportOOMError(conn); goto cleanup; } node = virXPathNode(conn, "/source", xpath_ctxt); if (!node) { virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s", _("root element was not source")); goto cleanup; } if (virStoragePoolDefParseSource(conn, xpath_ctxt, def, pool_type, node) < 0) goto cleanup; ret = def; def = NULL; cleanup: if (def) virStoragePoolSourceFree(def); <<<<======== yet not freed xmlFreeDoc(doc); xmlXPathFreeContext(xpath_ctxt); return ret; } One fix might be to call VIR_FREE(def) in the "if (def)..." block, but that seems short-sighted, since the name virStoragePoolSourceFree makes me think *it* should be doing the whole job. However, if I make the logical change to virStoragePoolSourceFree, diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 62b8394..ffe38cc 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -291,6 +291,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) { VIR_FREE(source->auth.chap.login); VIR_FREE(source->auth.chap.passwd); } + VIR_FREE(source); } that causes "make check" to fail miserably due to heap corruption, as reported by valgrind: ==30311== Invalid free() / delete / delete[] ==30311== at 0x4A04D72: free (vg_replace_malloc.c:325) ==30311== by 0x42950C: virFree (memory.c:177) ==30311== by 0x4A2687: virStoragePoolSourceFree (storage_conf.c:294) ==30311== by 0x4A26BD: virStoragePoolDefFree (storage_conf.c:304) ==30311== by 0x4A2723: virStoragePoolObjFree (storage_conf.c:319) ==30311== by 0x4A27A2: virStoragePoolObjListFree (storage_conf.c:334) ==30311== by 0x4757AD: storageDriverShutdown (storage_driver.c:246) ==30311== by 0x4C1E3F: virStateCleanup (libvirt.c:909) ==30311== by 0x4103C5: qemudCleanup (libvirtd.c:2402) ==30311== by 0x41273B: main (libvirtd.c:3196) ==30311== Address 0x5c48c38 is 56 bytes inside a block of size 184 alloc'd ==30311== at 0x4A04481: calloc (vg_replace_malloc.c:418) ==30311== by 0x4293F5: virAlloc (memory.c:100) ==30311== by 0x4A330E: virStoragePoolDefParseXML (storage_conf.c:615) ==30311== by 0x4A3A21: virStoragePoolDefParseNode (storage_conf.c:762) ==30311== by 0x4A3BE6: virStoragePoolDefParse (storage_conf.c:810) ==30311== by 0x4A3C7F: virStoragePoolDefParseFile (storage_conf.c:834) ==30311== by 0x4A5816: virStoragePoolObjLoad (storage_conf.c:1495) ==30311== by 0x4A5BDB: virStoragePoolLoadAllConfigs (storage_conf.c:1575) ==30311== by 0x475598: storageDriverStartup (storage_driver.c:164) ==30311== by 0x4C1D9D: virStateInitialize (libvirt.c:888) ==30311== by 0x4125E9: main (libvirtd.c:3153) I tracked the problem down to this definition in src/conf/storage_conf.h: typedef struct _virStoragePoolDef virStoragePoolDef; typedef virStoragePoolDef *virStoragePoolDefPtr; struct _virStoragePoolDef { /* General metadata */ char *name; unsigned char uuid[VIR_UUID_BUFLEN]; int type; /* virStoragePoolType */ unsigned long long allocation; unsigned long long capacity; unsigned long long available; virStoragePoolSource source; <== this is a *STRUCT*, not a ptr virStoragePoolTarget target; <== Likewise }; Hence, when calling virStoragePoolDefFree (defined like this) void virStoragePoolDefFree(virStoragePoolDefPtr def) { if (!def) return; VIR_FREE(def->name); virStoragePoolSourceFree(&def->source); That function must *not* call VIR_FREE on &def->source, because as valgrind tells us, above, that is in the middle of a virStoragePoolDef buffer. Thus, the naive patch seems to be the way to go, unless you want to change the struct members to be pointers (and adjust all uses -- much more invasive). There are other leaks associated with similar uses of virStoragePoolSourceFree just like this one. Here's a patch that fixes them all at once. If you agree in principle, I'll write a longer commit log than the one below: [note that there's no need to guard uses of virStoragePoolSourceFree against NULL args, so I removed those. ] >From a53efcee7ec74948a2cf4c1c02902419856b0154 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 5 Feb 2010 17:09:43 +0100 Subject: [PATCH] plug four virStoragePoolSourceFree-related leaks * src/conf/storage_conf.c (virStoragePoolDefParseSourceString): * src/storage/storage_backend_fs.c: (virStorageBackendFileSystemNetFindPoolSourcesFunc): (virStorageBackendFileSystemNetFindPoolSources): * src/test/test_driver.c (testStorageFindPoolSources): --- src/conf/storage_conf.c | 4 ++-- src/storage/storage_backend_fs.c | 8 ++++---- src/test/test_driver.c | 3 ++- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 62b8394..b98b8e9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -524,8 +524,8 @@ virStoragePoolDefParseSourceString(virConnectPtr conn, ret = def; def = NULL; cleanup: - if (def) - virStoragePoolSourceFree(def); + virStoragePoolSourceFree(def); + VIR_FREE(def); xmlFreeDoc(doc); xmlXPathFreeContext(xpath_ctxt); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0d1c7a7..70e91a0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -174,8 +174,8 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(virConnectPtr conn, src = NULL; ret = 0; cleanup: - if (src) - virStoragePoolSourceFree(src); + virStoragePoolSourceFree(src); + VIR_FREE(src); return ret; } @@ -236,8 +236,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn, for (i = 0; i < state.list.nsources; i++) virStoragePoolSourceFree(&state.list.sources[i]); - if (source) - virStoragePoolSourceFree(source); + virStoragePoolSourceFree(source); + VIR_FREE(source); VIR_FREE(state.list.sources); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2122a1b..8e5ecf2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1,7 +1,7 @@ /* * test.c: A "mock" hypervisor for use by application unit tests * - * Copyright (C) 2006-2009 Red Hat, Inc. + * Copyright (C) 2006-2010 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -3763,6 +3763,7 @@ testStorageFindPoolSources(virConnectPtr conn, cleanup: virStoragePoolSourceFree(source); + VIR_FREE(source); return ret; } -- 1.7.0.rc1.204.gb96e

2 2

[libvirt] [PATCH 1/5] macvtap support for libvirt -- build support
by Stefan Berger 11 Feb '10

11 Feb '10

This patch adds build support for libvirt checking for certain contents of /usr/include/linux/if_link.h to see whether macvtap support is compilable on that system. One can disable macvtap support in libvirt via --without-macvtap passed to configure. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>

3 2

[libvirt] Node devices?
by Ed Swierk 11 Feb '10

11 Feb '10

After staring at the code implementing node devices in libvirt, I have decided that they're really neat, but I have no idea what they are for or how to use them. Reading the API docs and searching libvir-list didn't help much, and the user documentation (http://libvirt.org/archnode.html and http://libvirt.org/formatnode.html) is a bit, err, high-level. Can someone enlighten me? --Ed

5 5