- Devel - libvirt List Archives

[libvirt] [PATCH] nodeinfo: skip offline CPUs
by Eric Blake 11 Aug '10

11 Aug '10

https://bugzilla.redhat.com/622515 - When hot-unplugging CPUs, libvirt failed to start a guest that had been pinned to CPUs that were still online, because it was failing to read status from unrelated offline CPUs. Tested on a dual-core laptop, where I also discovered that, per http://www.cyberciti.biz/files/linux-kernel/Documentation/cpu-hotplug.txt, /sys/devices/system/cpu/cpu0/online does not exist on systems where it cannot be hot-unplugged. * src/nodeinfo.c (linuxNodeInfoCPUPopulate): Ignore CPUs that are currently offline. Detect readdir failure. (parse_socket): Move guts... (get_cpu_value): ...to new function, shared with... (cpu_online): New function. --- src/nodeinfo.c | 109 +++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 73 insertions(+), 36 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 5ec1bcf..6286aa2 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -23,6 +23,7 @@ #include <config.h> +#include <stdbool.h> #include <stdio.h> #include <string.h> #include <stdlib.h> @@ -60,6 +61,58 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, virNodeInfoPtr nodeinfo); +/* Return the positive decimal contents of the given + * CPU_SYS_PATH/cpu%u/FILE, or -1 on error. If MISSING_OK and the + * file could not be found, return 1 instead of an error; this is + * because some machines cannot hot-unplug cpu0. */ +static int get_cpu_value(unsigned int cpu, const char *file, bool missing_ok) +{ + char *path; + FILE *pathfp; + char value_str[1024]; + char *tmp; + int value = -1; + + if (virAsprintf(&path, CPU_SYS_PATH "/cpu%u/%s", cpu, file) < 0) { + virReportOOMError(); + return -1; + } + + pathfp = fopen(path, "r"); + if (pathfp == NULL) { + if (missing_ok && errno == ENOENT) + value = 1; + else + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + + if (fgets(value_str, sizeof(value_str), pathfp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + if (virStrToLong_i(value_str, &tmp, 10, &value) < 0) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + _("could not convert '%s' to an integer"), + value_str); + goto cleanup; + } + +cleanup: + if (pathfp) + fclose(pathfp); + VIR_FREE(path); + + return value; +} + +/* Check if CPU is online via CPU_SYS_PATH/cpu%u/online. Return 1 if online, + 0 if offline, and -1 on error. */ +static int cpu_online(unsigned int cpu) +{ + return get_cpu_value(cpu, "online", cpu == 0); +} + static unsigned long count_thread_siblings(unsigned int cpu) { unsigned long ret = 0; @@ -106,41 +159,7 @@ cleanup: static int parse_socket(unsigned int cpu) { - char *path; - FILE *pathfp; - char socket_str[1024]; - char *tmp; - int socket = -1; - - if (virAsprintf(&path, CPU_SYS_PATH "/cpu%u/topology/physical_package_id", - cpu) < 0) { - virReportOOMError(); - return -1; - } - - pathfp = fopen(path, "r"); - if (pathfp == NULL) { - virReportSystemError(errno, _("cannot open %s"), path); - VIR_FREE(path); - return -1; - } - - if (fgets(socket_str, sizeof(socket_str), pathfp) == NULL) { - virReportSystemError(errno, _("cannot read from %s"), path); - goto cleanup; - } - if (virStrToLong_i(socket_str, &tmp, 10, &socket) < 0) { - nodeReportError(VIR_ERR_INTERNAL_ERROR, - _("could not convert '%s' to an integer"), - socket_str); - goto cleanup; - } - -cleanup: - fclose(pathfp); - VIR_FREE(path); - - return socket; + return get_cpu_value(cpu, "topology/physical_package_id", false); } int linuxNodeInfoCPUPopulate(FILE *cpuinfo, @@ -153,6 +172,8 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, unsigned long cur_threads; int socket; unsigned long long socket_mask = 0; + unsigned int remaining; + int online; nodeinfo->cpus = 0; nodeinfo->mhz = 0; @@ -222,15 +243,25 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, /* OK, we've parsed what we can out of /proc/cpuinfo. Get the socket * and thread information from /sys */ + remaining = nodeinfo->cpus; cpudir = opendir(CPU_SYS_PATH); if (cpudir == NULL) { virReportSystemError(errno, _("cannot opendir %s"), CPU_SYS_PATH); return -1; } - while ((cpudirent = readdir(cpudir))) { + while (errno = 0, remaining && (cpudirent = readdir(cpudir))) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; + online = cpu_online(cpu); + if (online < 0) { + closedir(cpudir); + return -1; + } + if (!online) + continue; + remaining--; + socket = parse_socket(cpu); if (socket < 0) { closedir(cpudir); @@ -249,6 +280,12 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, if (cur_threads > nodeinfo->threads) nodeinfo->threads = cur_threads; } + if (errno) { + virReportSystemError(errno, + _("problem reading %s"), CPU_SYS_PATH); + closedir(cpudir); + return -1; + } closedir(cpudir); -- 1.7.1

2 3

[libvirt] [PATCH] Avoid autogen loop in VPATH builds
by Jiri Denemark 11 Aug '10

11 Aug '10

--- cfg.mk | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/cfg.mk b/cfg.mk index 7226828..e7fd63c 100644 --- a/cfg.mk +++ b/cfg.mk @@ -459,7 +459,7 @@ ifeq (0,$(MAKELEVEL)) # b653eda3ac4864de205419d9f41eec267cb89eeb _submodule_hash = sed 's/^[ +-]//;s/ .*//' _update_required := $(shell \ - test -f po/Makevars || { echo 1; exit; }; \ + test -f $(srcdir)/po/Makevars || { echo 1; exit; }; \ cd '$(srcdir)'; \ actual=$$(git submodule status | $(_submodule_hash); \ git hash-object bootstrap.conf; \ -- 1.7.2

2 5

[libvirt] [PATCH 1/2] Fix return value usage
by Doug Goldstein 11 Aug '10

11 Aug '10

Fix the error checking to use the return value from brAddTap() instead of checking the current errno value which might have been changed by clean up calls inside of brAddTap(). Signed-off-by: Doug Goldstein <cardoe(a)gentoo.org> --- src/uml/uml_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index bef8c38..025169f 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -141,7 +141,7 @@ umlConnectTapDevice(virDomainNetDefPtr net, tapmac, 0, &tapfd))) { - if (errno == ENOTSUP) { + if (err == ENOTSUP) { /* In this particular case, give a better diagnostic. */ umlReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to add tap interface to bridge. " -- 1.7.2

1 0

[libvirt] [PATCH] qemu: Hack around asynchronous device_del
by Jiri Denemark 11 Aug '10

11 Aug '10

device_del command is not synchronous for PCI devices, it merely asks the guest to release the device and returns. If the host wants to use that device before the guest actually releases it, we are in big trouble. To avoid this, we already added a loop which waits up to 10 seconds until the device is actually released before we do anything else with that device. But we only added this loop for managed PCI devices before we try reattach them back to the host. However, we need to wait even for non-managed devices. We don't reattach them automatically, but we still want to prevent the host from using it. This was revealed thanks to sVirt: when we relabel sysfs files corresponding to the PCI device before the guest finished releasing the device, qemu is no longer allowed to access those files and if it wants (as a result of guest's request) to write anything to them, it just exits, which kills the guest. This is not a proper fix and needs some further work both on libvirt and qemu side in the future. --- I was thinking about another possibility to implement this since the wait loop doesn't have to be connected to the reattach action. We could move that loop into a separate function and let qemudReattach function do just what its name suggest. But in that case, we would need to add the call to such function before every call to qemudReattach, which doesn't look any better to me. Jirka src/qemu/qemu_driver.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57b8271..e4f47d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3209,16 +3209,17 @@ qemuPrepareChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, static void -qemudReattachManagedDevice(pciDevice *dev, struct qemud_driver *driver) +qemudReattachPciDevice(pciDevice *dev, struct qemud_driver *driver) { int retries = 100; + while (pciWaitForDeviceCleanup(dev, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + if (pciDeviceGetManaged(dev)) { - while (pciWaitForDeviceCleanup(dev, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } if (pciReAttachDevice(dev, driver->activePciHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s"), @@ -3264,7 +3265,7 @@ qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - qemudReattachManagedDevice(dev, driver); + qemudReattachPciDevice(dev, driver); } pciDeviceListFree(pcidevs); @@ -8997,7 +8998,7 @@ static int qemudDomainDetachHostPciDevice(struct qemud_driver *driver, pciDeviceListDel(driver->activePciHostdevs, pci); if (pciResetDevice(pci, driver->activePciHostdevs, NULL) < 0) ret = -1; - qemudReattachManagedDevice(pci, driver); + qemudReattachPciDevice(pci, driver); pciFreeDevice(pci); } -- 1.7.2

2 2

[libvirt] Libvirt storage api and lvm .cache issue
by Thomas Graves 10 Aug '10

10 Aug '10

Hello all, I am using the libvirt storage api with LVM storage on rhel5 with xen and I noticed that when using the api to create/delete lv's they get added to /etc/lvm/cache/.cache but old deleted ones never get removed from the .cache. This causes dmeventd to keep trying to access them but can't which fills up the logs in /var/log. Has anyone seen this or have a fix for it? I guess I could force a vgscan or something to clean it up just seems like I shouldn't need to. Thanks, Tom

2 3

[libvirt] [PATCH] Add "ubd" to the list of disk prefixes
by Soren Hansen 10 Aug '10

10 Aug '10

virDiskNameToIndex has a list of disk name prefixes that it uses in the process of finding the disk's index. This list is missing "ubd" which is the disk prefix used for UML domains. Signed-off-by: Soren Hansen <soren(a)linux2go.dk> --- src/util/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 8f2a17e..c173e49 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2367,7 +2367,7 @@ const char *virEnumToString(const char *const*types, int virDiskNameToIndex(const char *name) { const char *ptr = NULL; int idx = 0; - static char const* const drive_prefix[] = {"fd", "hd", "vd", "sd", "xvd"}; + static char const* const drive_prefix[] = {"fd", "hd", "vd", "sd", "xvd", "ubd"}; unsigned int i; for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) { -- 1.7.0.4

3 4

[libvirt] [PATCH] build-sys: only build the test programs during the check phase.
by Diego Elio Pettenò 10 Aug '10

10 Aug '10

This avoids building the tests when testing libvirt is not the aim. --- tests/Makefile.am | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 71c2c74..1dc7f66 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -72,42 +72,42 @@ EXTRA_DIST = \ domainsnapshotxml2xmlin \ qemuhelpdata -noinst_PROGRAMS = virshtest conftest \ +check_PROGRAMS = virshtest conftest \ nodeinfotest statstest qparamtest if WITH_XEN -noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \ +check_PROGRAMS += xml2sexprtest sexpr2xmltest \ reconnect xmconfigtest xencapstest endif if WITH_QEMU -noinst_PROGRAMS += qemuxml2argvtest qemuxml2xmltest qemuargv2xmltest qemuhelptest +check_PROGRAMS += qemuxml2argvtest qemuxml2xmltest qemuargv2xmltest qemuhelptest endif if WITH_ESX -noinst_PROGRAMS += esxutilstest vmx2xmltest xml2vmxtest +check_PROGRAMS += esxutilstest vmx2xmltest xml2vmxtest endif if WITH_SECDRIVER_SELINUX -noinst_PROGRAMS += seclabeltest +check_PROGRAMS += seclabeltest endif if WITH_SECDRIVER_APPARMOR -noinst_PROGRAMS += secaatest +check_PROGRAMS += secaatest endif if WITH_CIL -noinst_PROGRAMS += object-locking +check_PROGRAMS += object-locking endif -noinst_PROGRAMS += networkxml2xmltest +check_PROGRAMS += networkxml2xmltest -noinst_PROGRAMS += nwfilterxml2xmltest +check_PROGRAMS += nwfilterxml2xmltest -noinst_PROGRAMS += storagevolxml2xmltest storagepoolxml2xmltest +check_PROGRAMS += storagevolxml2xmltest storagepoolxml2xmltest -noinst_PROGRAMS += nodedevxml2xmltest +check_PROGRAMS += nodedevxml2xmltest -noinst_PROGRAMS += interfacexml2xmltest +check_PROGRAMS += interfacexml2xmltest test_scripts = \ capabilityschematest \ @@ -181,7 +181,7 @@ TESTS += secaatest endif if WITH_LIBVIRTD -noinst_PROGRAMS += eventtest +check_PROGRAMS += eventtest TESTS += eventtest endif -- 1.7.2

2 1

[libvirt] [PATCH 1/2] util: add wrapper function to check if address string is an ip
by Justin Clift 10 Aug '10

10 Aug '10

--- src/util/network.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/network.h | 2 ++ 2 files changed, 39 insertions(+), 0 deletions(-) diff --git a/src/util/network.c b/src/util/network.c index 6e24792..2e0cf9c 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -54,6 +54,43 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { } /** + * virIsNumericIPAddr: + * @address: a text string containing a host name or IPv4/IPv6 address + * + * Given a text string, determines whether it contains a valid IPv4/IPv6 + * address. + * + * Returns 0 if the given string is an IPv4 or IPv6 address, or non-zero + * for any other condition. + */ +int +virIsNumericIPAddr(const char *address) { + int returnCode; + struct addrinfo hints; + struct addrinfo *res = NULL; + + /* Sanity check we've been given a text string */ + if (address == NULL) + return(-1); + + /* Do a name lookup on the given string, and see what we're given back */ + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_UNSPEC; /* Doesn't matter whether IPv4 or IPv6 */ + hints.ai_socktype = 0; /* Socket type doesn't matter */ + hints.ai_flags = AI_NUMERICHOST; /* Force a numeric IP address check */ + hints.ai_protocol = 0; /* Doesn't matter which protocol */ + returnCode = getaddrinfo (address, NULL, &hints, &res); + if (0 == returnCode) { + /* A result was returned. This means the text string we were + * given is a valid IP address. We now free the result(s) we + * were given, and pass back the return code */ + freeaddrinfo(res); + } + + return returnCode; +} + +/** * virSocketParseAddr: * @val: a numeric network address IPv4 or IPv6 * @addr: where to store the return value. diff --git a/src/util/network.h b/src/util/network.h index 5307c8c..c2e88d8 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -24,6 +24,8 @@ typedef union { } virSocketAddr; typedef virSocketAddr *virSocketAddrPtr; +int virIsNumericIPAddr (const char *address); + int virSocketParseAddr (const char *val, virSocketAddrPtr addr, int hint); -- 1.7.2.1

3 7

Re: [libvirt] [PATCH] allow memballoon type of none to desactivate it
by Daniel P. Berrange 10 Aug '10

10 Aug '10

On Tue, Aug 10, 2010 at 12:09:47AM +0200, Daniel Veillard wrote: > Mail disapeared when trying to send it... sending again ... > > On Mon, Aug 09, 2010 at 07:38:32PM +0100, Daniel P. Berrange wrote: > > On Mon, Aug 09, 2010 at 08:34:23PM +0200, Daniel Veillard wrote: > > > Grumpf ... that mean at the internal stucture level we need to add an > > > extra field, that is detected at a completely different time in parsing > > > too ... more complex in general, but I can understand the purity POV. > > > > I don't see this as a real problem. It is no different from the way that > > we automatically add <controller> devices at the end of parsing, if we saw > > any <disk> or <channel> devices previously. > > Huh ??? You can have a controller without a disk and you don't > automagically add a controller even if nothing was suggested by the > user, it is widely different. First you need to keep a tristate for > def->balloonable, was that attribute defined, or not, then add the > error case if it's not "yes" or "no", then you need to care for > all the different cases of the tristate when you actually parse > the devices section, how do you handle <memballon> definition when > balloonable="no" was found, discard, error ? We'd raise an error becasue the requested config doesn't make sense. It doesn't actually need to be a tri-state - ballooning is either enabled or not. If a user hasn't specified which in the XML, then we'd pick the hypervisor default for that. > I find this different because you provide the information in 2 places > at different levels and you ned to cope with conflict ... while still > maintaining the current behaviour of autoadding. > This then breaks nearly all the qemu XML tests due to the extra > balloonable="yes" added automatically to the output since the qemu > driver makes that a default. Yep, that's an unavoidable side-effect of adding more default data to our XML output, but we've hit this many times in the past. eg adding disk controllers, required us to change every single test datafile. > And now you have to explain in documentation all the various cases > instead of a simple one liner about using "none". > > When data is defined at 2 different places by design, well you designed > a mess. And in that case that's cerainly true. And after looking at the > situation in detail, no I don't by the "purity" point of view, this > actually make things *harder* both for user and from a implementation > point of view. > I firmly think that in that case the proper way to do this is to > keep the information in one place and that place is the <memballoon> > device, type='none' may look ugly to you, but at least the behaviour > will be predictable, users will know where to look and that can be > explained to the in a single line/sentence. Neither option is keeping all the information in one place. We already have a four-way split of memory information with <memory> vs <currentMemory>, <memoryBacking> and now <memballoon>. The first three are defining attributes of the VM itself, while the latter is defining information about the device - specifically we added it to provide info about PCI device IDs & the device alias. Whether a guest domain supports ballooning or not is an attribute of the domain itself. If there is no balloon, then there is no device to provide information on, so none of the data that appears under <memballoon> even exists, nor can you pass this device element to the hotplug APIs, because there's no actal device behind it. This makes the <memballoon> element non-sensical to me. > Current patch provided, I strongly suggest to not continue down this > line and apply the previous patch instead. To be fair, I don't actually like either patch, I'm just going for what I think is the least worst option long term. If we could go back in time, I'd never have enabled the balloon device by default in the first place and required an explicit <memballoon>. I'd actually prefer to just ignore this problem completely, and declare that all KVM guest must always have a balloon and not provide any way to disable it. The issue of limited PCI slots is really a QEMU flaw - it should be providing a PCI bridge support so you can have a much less limited number of PCI slots. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

2 1

[libvirt] [PATCH] bootloader_args is named bootargs in xen-xm
by Philipp Hahn 10 Aug '10

10 Aug '10

According to <xen-3.4.3/tools/python/xen/xm/create.py:158> gopts.var('bootargs', val='NAME', fn=set_value, default=None, use="Arguments to pass to boot loader") the "bootloader_args" parameter needs to be translated into "bootargs" when using "virsh domxml-to-native xen-xm". The reverse direction (domxml-from-native) is already okay. This patch fixes domxml-to-native and adds two test files to catch this problem. Signed-off-by: Philipp Hahn <hahn(a)univention.de> --- src/xen/xm_internal.c | 2 +- tests/xmconfigdata/sexpr2xml-pv-bootloader.cfg | 12 +++++++++++ tests/xmconfigdata/sexpr2xml-pv-bootloader.xml | 26 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletions(-) create mode 100644 tests/xmconfigdata/sexpr2xml-pv-bootloader.cfg create mode 100644 tests/xmconfigdata/sexpr2xml-pv-bootloader.xml

2 1