[libvirt] virStoragePoolSourceFree(S) does not free S
by Jim Meyering
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
14 years, 9 months
[libvirt] [PATCH 1/5] macvtap support for libvirt -- build support
by Stefan Berger
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>
14 years, 9 months
[libvirt] [PATCH] Fix disk stats retrieval with QEMU >= 0.12
by Daniel P. Berrange
With QEMU >= 0.12 the host and guest side of disks no longer have
the same naming convention. Specifically the host side will now
get a 'drive-' prefix added to its name. The 'info blockstats'
monitor command returns the host side name, so it is neccessary
to strip this off when looking up stats since libvirt stores the
guest side name !
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Move 'drive-' prefix
string to a defined constant
* src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_text.c: Strip
off 'drive-' prefix (if found) when looking up disk stats
---
src/qemu/qemu_conf.c | 4 ++--
src/qemu/qemu_conf.h | 2 ++
src/qemu/qemu_monitor_json.c | 7 +++++++
src/qemu/qemu_monitor_text.c | 7 +++++++
4 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3988582..c1d03cd 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -2301,7 +2301,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
virBufferAddLit(&opt, ",media=cdrom");
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) {
- virBufferVSprintf(&opt, ",id=drive-%s", disk->info.alias);
+ virBufferVSprintf(&opt, ",id=%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias);
} else {
if (busid == -1 && unitid == -1) {
if (idx != -1)
@@ -2390,7 +2390,7 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk)
_("unsupported disk bus '%s' with device setup"), bus);
goto error;
}
- virBufferVSprintf(&opt, ",drive=drive-%s", disk->info.alias);
+ virBufferVSprintf(&opt, ",drive=%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias);
virBufferVSprintf(&opt, ",id=%s", disk->info.alias);
if (virBufferError(&opt)) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index d26bb90..498f1d1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -157,6 +157,8 @@ typedef qemuDomainPCIAddressSet *qemuDomainPCIAddressSetPtr;
/* Config type for XML import/export conversions */
#define QEMU_CONFIG_FORMAT_ARGV "qemu-argv"
+#define QEMU_DRIVE_HOST_PREFIX "drive-"
+
#define qemuReportError(code, fmt...) \
virReportErrorHelper(NULL, VIR_FROM_QEMU, code, __FILE__, \
__FUNCTION__, __LINE__, fmt)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e5288ce..032afef 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -754,6 +754,13 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
goto cleanup;
}
+ /* New QEMU has separate names for host & guest side of the disk
+ * and libvirt gives the host side a 'drive-' prefix. The passed
+ * in devname is the guest side though
+ */
+ if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX))
+ thisdev += strlen(QEMU_DRIVE_HOST_PREFIX);
+
if (STRNEQ(thisdev, devname))
continue;
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 23fc4cf..a6a4598 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -649,6 +649,13 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon,
p = info;
while (*p) {
+ /* New QEMU has separate names for host & guest side of the disk
+ * and libvirt gives the host side a 'drive-' prefix. The passed
+ * in devname is the guest side though
+ */
+ if (STRPREFIX(p, QEMU_DRIVE_HOST_PREFIX))
+ p += strlen(QEMU_DRIVE_HOST_PREFIX);
+
if (STREQLEN (p, devname, devnamelen)
&& p[devnamelen] == ':' && p[devnamelen+1] == ' ') {
--
1.6.5.2
14 years, 9 months
[libvirt] domblkstat not working for qemu-kvm > 0.11
by Thomas Treutner
Hi,
with all qemu-kvm > 0.11, virsh domblkstat is not working:
virsh -c qemu://node03/system domblkstat wp01 vdb : invalid argument in no
stats found for device virtio-disk1
(vda, vdb etc. works with 0.11)
"virtio-disk1" seems to be a red herring:
virsh -c qemu://node03/system domblkstat wp01 virtio-disk1 : invalid argument
in invalid path: virtio-disk1
Most probably there have been made some changes with 0.12 libvirt does not
resemble yet. If you need more debugging output, just tell.
kr,t
14 years, 9 months
[libvirt] [PATCH] qemu: Properly report a startup timeout error
by Cole Robinson
The timeout errors were unconditionally being overwritten by the less
helpful 'unable to start guest' error.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/qemu/qemu_driver.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4374291..2172c99 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1358,15 +1358,15 @@ qemudReadLogOutput(virConnectPtr conn,
buf[got] = '\0';
if (got == buflen-1) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
- _("Out of space while reading %s log output"),
- what);
+ _("Out of space while reading %s log output: %s"),
+ what, buf);
return -1;
}
if (isdead) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
- _("Process exited while reading %s log output"),
- what);
+ _("Process exited while reading %s log output: %s"),
+ what, buf);
return -1;
}
@@ -1378,7 +1378,8 @@ qemudReadLogOutput(virConnectPtr conn,
}
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
- _("Timed out while reading %s log output"), what);
+ _("Timed out while reading %s log output: %s"),
+ what, buf);
return -1;
}
@@ -1557,12 +1558,8 @@ qemudWaitForMonitor(virConnectPtr conn,
virStrerror(errno, ebuf, sizeof ebuf));
}
- if (ret < 0) {
- /* Unexpected end of file - inform user of QEMU log data */
- qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
- _("unable to start guest: %s"), buf);
+ if (ret < 0)
return -1;
- }
VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name);
if (qemuConnectMonitor(vm) < 0)
--
1.6.5.2
14 years, 9 months
[libvirt] [PATCH] test: Fake security driver support in capabilities
by Cole Robinson
Having some value in capabilities helps testing this stuff in
virt-manager.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/test/test_driver.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 411c5cd..3f95a22 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -205,6 +205,14 @@ testBuildCapabilities(virConnectPtr conn) {
caps->privateDataAllocFunc = testDomainObjPrivateAlloc;
caps->privateDataFreeFunc = testDomainObjPrivateFree;
+ caps->host.secModel.model = strdup("testSecurity");
+ if (!caps->host.secModel.model)
+ goto no_memory;
+
+ caps->host.secModel.doi = strdup("");
+ if (!caps->host.secModel.doi)
+ goto no_memory;
+
return caps;
no_memory:
--
1.6.5.2
14 years, 9 months
[libvirt] [PATCH] (absolutePathFromBaseFile): fix up preceding commit
by Jim Meyering
To my chagrin, I saw that my most recent commit introduced
compilation errors. Sorry about that.
Here's how I propose to fix it.
>From 2d948a373ecebec6c06274f61b31d1ae9c40ae41 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Fri, 5 Feb 2010 14:57:35 +0100
Subject: [PATCH] (absolutePathFromBaseFile): fix up preceding commit
* src/util/storage_file.c: Include <assert.h>.
(absolutePathFromBaseFile): Assert that converting size_t to int is valid.
Reverse length/string args to match "%.*s".
Explicitly ignore the return value of virAsprintf.
---
src/util/storage_file.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 2c79fa9..135acec 100644
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -26,7 +26,9 @@
#include <unistd.h>
#include <fcntl.h>
+#include <assert.h>
#include "dirname.h"
+#include "ignore-value.h"
#include "memory.h"
#include "virterror_internal.h"
@@ -255,7 +257,10 @@ absolutePathFromBaseFile(const char *base_file, const char *path)
if (*path == '/' || d_len == 0)
return strdup(path);
- virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
+ /* Ensure that the following cast-to-int is valid. */
+ assert (d_len <= INT_MAX);
+
+ ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path));
return res;
}
--
1.7.0.rc1.204.gb96e
14 years, 9 months
