February 2010 - Devel - libvirt List Archives

[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 10 Feb '10

10 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

[libvirt] [PATCH] Fix disk stats retrieval with QEMU >= 0.12
by Daniel P. Berrange 10 Feb '10

10 Feb '10

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

3 2

[libvirt] domblkstat not working for qemu-kvm > 0.11
by Thomas Treutner 10 Feb '10

10 Feb '10

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

2 2

[libvirt] [PATCH] qemu: Properly report a startup timeout error
by Cole Robinson 10 Feb '10

10 Feb '10

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

3 6

[libvirt] [PATCH] test: Fake security driver support in capabilities
by Cole Robinson 10 Feb '10

10 Feb '10

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

2 2

[libvirt] [PATCH] (absolutePathFromBaseFile): fix up preceding commit
by Jim Meyering 10 Feb '10

10 Feb '10

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

3 4