[libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure
I've just begun using clang's static analyzer, http://clang-analyzer.llvm.org/ It has uncovered a few problems in libvirt. Here are the first few fixes. I'll send more details later today. From b6bb9d82effa56733fbee9013e66fed384d9ff63 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 09:42:32 +0200 Subject: [PATCH 1/4] storage_backend_fs: avoid NULL dereference on opendir failure * src/storage_backend_fs.c (virStorageBackendFileSystemRefresh): Don't call closedir on a NULL pointer. --- src/storage_backend_fs.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 65b656d..8241504 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -983,7 +983,8 @@ no_memory: /* fallthrough */ cleanup: -closedir(dir); +if (dir) +closedir(dir); virStorageVolDefFree(vol); virStoragePoolObjClearVols(pool); return -1; -- 1.6.4.2.395.ge3d52 From eaae148291680a72d19aa9d5320f90b98f123746 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 09:58:28 +0200 Subject: [PATCH 2/4] storage_conf.c: avoid overflow upon use of z or Z (zebi) suffix * src/storage_conf.c (virStorageSize): Don't try to compute 1024^7, since it's too large for a 64-bit type. --- src/storage_conf.c |6 -- 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/src/storage_conf.c b/src/storage_conf.c index c446069..110f0ad 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -919,12 +919,6 @@ virStorageSize(virConnectPtr conn, 1024ull; break; -case 'z': -case 'Z': -mult = 1024ull * 1024ull * 1024ull * 1024ull * 1024ull * -1024ull * 1024ull; -break; - default: virStorageReportError(conn, VIR_ERR_XML_ERROR, _(unknown size units '%s'), unit); -- 1.6.4.2.395.ge3d52 From 7f453c68bc709d542e4c40a388c92c7969ad0a3a Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 09:58:50 +0200 Subject: [PATCH 3/4] lxc: avoid NULL dereference when we find no mount point * src/lxc_container.c (lxcContainerUnmountOldFS): Don't pass a NULL pointer to qsort. --- src/lxc_container.c |5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lxc_container.c b/src/lxc_container.c index 950dd50..2073864 100644 --- a/src/lxc_container.c +++ b/src/lxc_container.c @@ -546,8 +546,9 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt); -qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); +if (mounts) +qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort); for (i = 0 ; i nmounts ; i++) { VIR_DEBUG(Umount %s, mounts[i]); -- 1.6.4.2.395.ge3d52 From 4e97befca175af427ed3b75f59e67cd620ee3ce2 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 10:02:49 +0200 Subject: [PATCH 4/4] lxc: don't unlink(NULL) in main * src/lxc_controller.c (main): Unlink sockpath only if it's non-NULL. --- src/lxc_controller.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/lxc_controller.c b/src/lxc_controller.c index 8d11238..914c10a 100644 --- a/src/lxc_controller.c +++ b/src/lxc_controller.c @@ -803,7 +803,8 @@ cleanup: if (def) virFileDeletePid(LXC_STATE_DIR, def-name); lxcControllerCleanupInterfaces(nveths, veths); -unlink(sockpath); +if (sockpath): +unlink(sockpath); VIR_FREE(sockpath); return rc; -- 1.6.4.2.395.ge3d52 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure
On Wed, Sep 02, 2009 at 10:05:03AM +0200, Jim Meyering wrote: I've just begun using clang's static analyzer, http://clang-analyzer.llvm.org/ It has uncovered a few problems in libvirt. Here are the first few fixes. I'll send more details later today. All patches look fine, it's just too bad we have to drop the 'z' storage size suffix, but it's not realistic on current platforms. ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] Add helper module for dealing with USB host devices
On Tue, Sep 01, 2009 at 04:28:54PM +0100, Daniel P. Berrange wrote: * src/Makefile.am: Add usb.h and usb.h to libvirt_util.la * src/libvirt_private.syms: Export symbols * src/usb.c, src/usb.h: Helper APIs for USB host devices --- src/Makefile.am |1 + src/hostusb.c| 103 ++ src/hostusb.h| 45 src/libvirt_private.syms |4 ++ 4 files changed, 153 insertions(+), 0 deletions(-) create mode 100644 src/hostusb.c create mode 100644 src/hostusb.h Sounds good [...] +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE VIR_FROM_STORAGE might be a bit more precise +usbDevice * +usbGetDevice(virConnectPtr conn, + unsigned bus, + unsigned devno) +{ The function code should probably be #ifdef linux +usbDevice *dev; + +if (VIR_ALLOC(dev) 0) { +virReportOOMError(conn); +return NULL; +} + +dev-bus = bus; +dev-dev = devno; + +snprintf(dev-name, sizeof(dev-name), %.3o:%.3o, + dev-bus, dev-dev); +snprintf(dev-path, sizeof(dev-path), + USB_DEVFS %03o/%03o, dev-bus, dev-dev); + +/* XXX fixme. this should be product/vendor */ +snprintf(dev-id, sizeof(dev-id), %d %d, dev-bus, dev-dev); + +VIR_DEBUG(%s %s: initialized, dev-id, dev-name); + +return dev; with a #else emitting an error Those tiny things apart, ACK, looks fine ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] Add helper APIs for iterating over PCI device resource files
On Tue, Sep 01, 2009 at 04:28:55PM +0100, Daniel P. Berrange wrote: [...] @@ -1022,3 +1022,55 @@ pciDeviceListFind(pciDeviceList *list, pciDevice *dev) return list-devs[i]; return NULL; } + + +int pciDeviceFileIterate(virConnectPtr conn, + pciDevice *dev, + pciDeviceFileActor actor, + void *opaque) +{ +char *pcidir = NULL; +char *file = NULL; +DIR *dir = NULL; +int ret = -1; +struct dirent *ent; + +if (virAsprintf(pcidir, /sys/bus/pci/devices/%04x:%02x:%02x.%x, +dev-domain, dev-bus, dev-slot, dev-function) 0) { hum %s/devices/%04x:%02x:%02x.%x , PCI_SYSFS, ... would be a bit better I guess Fine, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] Port QEMU driver to use USB/PCI device helpers
On Tue, Sep 01, 2009 at 04:28:56PM +0100, Daniel P. Berrange wrote: * src/qemu_driver.c: Remove usbfs/sysfs iterator code and call into generic helper APIs instead when setting device permissions Yup, nice cleanup, ACK, just one thing, it would be good in 1/6 and 2/6 to detail as a comment on the iterators in the headers the expected return value and the fact that non-zero will break iteration and be the returned value from top level. Usually when having to program those callbacks it's the classic pitfall :) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn
Here's the test just before the else-if in the patch below: if (conn conn-driver STREQ (conn-driver-name, remote)) { So, in the else-branch, conn is guaranteed to be NULL. And dereferenced. This may be only a theoretical risk, but if so, the test of conn above should be changed to an assertion, and/or the parameter should get the nonnull attribute. From a1b1d36d96f6b50ddf514539af85da20ca671bf5 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 11:54:38 +0200 Subject: [PATCH] remote_internal.c: don't dereference a NULL conn * src/remote_internal.c (remoteDevMonOpen): Avoid NULL-dereference. --- src/remote_internal.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/remote_internal.c b/src/remote_internal.c index ea50c11..141fef9 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -5148,7 +5148,7 @@ remoteDevMonOpen(virConnectPtr conn, conn-devMonPrivateData = priv; remoteDriverUnlock(priv); return VIR_DRV_OPEN_SUCCESS; -} else if (conn-networkDriver +} else if (conn conn-networkDriver STREQ (conn-networkDriver-name, remote)) { struct private_data *priv = conn-networkPrivateData; remoteDriverLock(priv); -- 1.6.4.2.395.ge3d52 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] Support relabelling of USB and PCI devices
On Tue, Sep 01, 2009 at 04:28:57PM +0100, Daniel P. Berrange wrote: * src/security.h: Driver API for relabelling host devices * src/security_selinux.c: Implement relabelling of PCI and USB devices * src/qemu_driver.c: Relabel USB/PCI devices before hotplug --- src/qemu_driver.c | 12 ++- src/security.h |7 ++ src/security_selinux.c | 175 +++- 3 files changed, 174 insertions(+), 20 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index e9a09df..d75e28e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5498,6 +5498,9 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) 0) return -1; +if (driver-securityDriver +driver-securityDriver-domainSetSecurityHostdevLabel(conn, vm, dev-data.hostdev) 0) +return -1; shouldn't we test against the entry point being null ? if (driver-securityDriver driver-securityDriver-domainSetSecurityHostdevLabel driver-securityDriver-domainSetSecurityHostdevLabel(conn, vm, dev-data.hostdev) 0) Also the long line should probably be split. [...] +if (driver-securityDriver +driver-securityDriver-domainSetSecurityHostdevLabel(conn, vm, dev-data.hostdev) 0) +VIR_WARN0(Failed to restore device labelling); + idem [...] static int +SELinuxRestoreSecurityImageLabel(virConnectPtr conn, + virDomainDiskDefPtr disk) +{ +/* Don't restore labels on readoly/shared disks, because typo readonly + * other VMs may still be accessing these + * Alternatively we could iterate over all running + * domains and try to figure out if it is in use, but + * this would not work for clustered filesystems, since + * we can't see running VMs using the file on other nodes + * Safest bet is thus to skip the restore step. + */ +if (disk-readonly || disk-shared) +return 0; + +if (!disk-src) +return 0; + +return SELinuxRestoreSecurityFileLabel(conn, disk-src); +} + +static int SELinuxSetSecurityImageLabel(virConnectPtr conn, virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -414,6 +427,126 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn, return 0; } + +static int +SELinuxSetSecurityPCILabel(virConnectPtr conn, + pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ +virDomainObjPtr vm = opaque; +const virSecurityLabelDefPtr secdef = vm-def-seclabel; + +return SELinuxSetFilecon(conn, file, secdef-imagelabel); +} it would be nice if we could check the type of the opaque passed in some ways, even if we know it was called a few line below. +static int +SELinuxSetSecurityHostdevLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainHostdevDefPtr dev) + +{ +int ret = -1; + +if (dev-mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) +return 0; + +switch (dev-source.subsys.type) { +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: +break; + +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { +pciDevice *pci = pciGetDevice(conn, + dev-source.subsys.u.pci.domain, + dev-source.subsys.u.pci.bus, + dev-source.subsys.u.pci.slot, + dev-source.subsys.u.pci.function); + +if (!pci) +goto done; + +ret = pciDeviceFileIterate(conn, pci, SELinuxSetSecurityPCILabel, vm); +pciFreeDevice(conn, pci); + +break; +} + +default: +ret = 0; +break; +} + +done: +return ret; +} Looks fine, ACK, but I won't pretend I understand all the implications of the security calls though, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn
On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote: Here's the test just before the else-if in the patch below: if (conn conn-driver STREQ (conn-driver-name, remote)) { So, in the else-branch, conn is guaranteed to be NULL. And dereferenced. This may be only a theoretical risk, but if so, the test of conn above should be changed to an assertion, and/or the parameter should get the nonnull attribute. I'm fine with your patch, so that even if code around changes having a gard is IMHO a good thing ACK thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] Support for CPUID masking
Hi, We need to provide support for CPU ID masking. Xen and VMware ESX are examples of current hypervisors which support such masking. My proposal is to define new 'cpuid' feature advertised in guest capabilities: guest ... features cpuid/ /feature /guest When a driver supports cpuid feature, one can use it to mask/check for specific bits returned by CPU ID as follows: domain ... features cpuid mask level='hex' register='eax|ebx|ecx|edx'MASK/mask /cpuid /features ... /domain Where - level is a hexadecimal number used as an input to CPU ID (i.e. eax register), - register is one of eax, ebx, ecx or edx, - and MASK is a string with the following format: ::::::: with m being 1-bit mask for the corresponding bit in the register. There are three possibilities of specifying what values can be used for 'm': - let it be driver-specific, - define all possible values, - define a common set of values and allow drivers to specify their own additional values. I think the third is the way to go as it lowers the confusion of different values used by different drivers for the same purpose while maintaining the flexibility to support driver-specific masks. The following could be a good set of predefined common values: - 1 force the bit to be 1 - 0 force the bit to be 0 - x don't care, i.e., use driver's default value - T require the bit to be 1 - F require the bit to be 0 Some examples of what it could look like follow: capabilities ... guest os_typexen/os_type ... features pae/ cpuid/ /features /guest ... /capabilities domain type='xen' id='42' ... features pae/ acpi/ apic/ cpuid mask level='1' register='ebx' :::1010:::: /mask mask level='1' register='ecx' ::::::xx1x: /mask mask level='1' register='edx' xxx1::::::: /mask mask level='8001' register='ecx' :::::::xx1x /mask mask level='8008' register='ecx' ::::::xx00:1001 /mask /cpuid /features ... /domain What are your opinions about this? Thank for all comments. Jirka -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn
Daniel Veillard wrote: On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote: Here's the test just before the else-if in the patch below: if (conn conn-driver STREQ (conn-driver-name, remote)) { So, in the else-branch, conn is guaranteed to be NULL. And dereferenced. This may be only a theoretical risk, but if so, the test of conn above should be changed to an assertion, and/or the parameter should get the nonnull attribute. I'm fine with your patch, so that even if code around changes having a gard is IMHO a good thing Thanks for the quick review. Daniel Berrange said he'd prefer the nonnull approach I mentioned above, so I've just adjusted (caveat, still not tested or even compiled) From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 12:20:32 +0200 Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters * src/internal.h (ATTRIBUTE_NONNULL): Define. --- src/internal.h |8 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/src/internal.h b/src/internal.h index 936cd03..8fa579c 100644 --- a/src/internal.h +++ b/src/internal.h @@ -116,6 +116,14 @@ #endif #endif +#ifndef ATTRIBUTE_NONNULL +# if __GNUC_PREREQ (3, 3) +# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) +# else +# define ATTRIBUTE_NONNULL(m) +# endif +#endif + #else #ifndef ATTRIBUTE_UNUSED #define ATTRIBUTE_UNUSED -- 1.6.4.2.395.ge3d52 From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 12:22:14 +0200 Subject: [PATCH 2/2] remote_internal.c: appease clang * src/remote_internal.c (remoteNetworkOpen): Mark conn parameter as non-NULL. Remove now-unnecessary conn == NULL test. (remoteDevMonOpen): Likewise. --- src/remote_internal.c | 10 -- 1 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/remote_internal.c b/src/remote_internal.c index ea50c11..fe36ddd 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3281,13 +3281,12 @@ done: static virDrvOpenStatus remoteNetworkOpen (virConnectPtr conn, virConnectAuthPtr auth, - int flags) + int flags) ATTRIBUTE_NONNULL (1) { if (inside_daemon) return VIR_DRV_OPEN_DECLINED; -if (conn -conn-driver +if (conn-driver STREQ (conn-driver-name, remote)) { struct private_data *priv; @@ -5130,13 +5129,12 @@ done: static virDrvOpenStatus remoteDevMonOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED) + int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1) { if (inside_daemon) return VIR_DRV_OPEN_DECLINED; -if (conn -conn-driver +if (conn-driver STREQ (conn-driver-name, remote)) { struct private_data *priv = conn-privateData; /* If we're here, the remote driver is already -- 1.6.4.2.395.ge3d52 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn
Jim Meyering wrote: Daniel Veillard wrote: On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote: Here's the test just before the else-if in the patch below: if (conn conn-driver STREQ (conn-driver-name, remote)) { So, in the else-branch, conn is guaranteed to be NULL. And dereferenced. This may be only a theoretical risk, but if so, the test of conn above should be changed to an assertion, and/or the parameter should get the nonnull attribute. I'm fine with your patch, so that even if code around changes having a gard is IMHO a good thing Thanks for the quick review. Daniel Berrange said he'd prefer the nonnull approach I mentioned above, so I've just adjusted (caveat, still not tested or even compiled) From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 12:20:32 +0200 Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters * src/internal.h (ATTRIBUTE_NONNULL): Define. ... From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 12:22:14 +0200 Subject: [PATCH 2/2] remote_internal.c: appease clang * src/remote_internal.c (remoteNetworkOpen): Mark conn parameter as non-NULL. Remove now-unnecessary conn == NULL test. (remoteDevMonOpen): Likewise. Note that this patch now address two reported NULL-deref problems, not just one, like the original. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn
On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote: Daniel Veillard wrote: On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote: Here's the test just before the else-if in the patch below: if (conn conn-driver STREQ (conn-driver-name, remote)) { So, in the else-branch, conn is guaranteed to be NULL. And dereferenced. This may be only a theoretical risk, but if so, the test of conn above should be changed to an assertion, and/or the parameter should get the nonnull attribute. I'm fine with your patch, so that even if code around changes having a gard is IMHO a good thing Thanks for the quick review. Daniel Berrange said he'd prefer the nonnull approach I mentioned above, so I've just adjusted (caveat, still not tested or even compiled) Yeah, for functions where it is expected that the passed in param be non-NULL, then annotations are definitely the way togo. This lets the compiler/checkers validate the callers instead, avoiding the need to clutter the methods with irrelevant NULL checks. From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 12:20:32 +0200 Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters * src/internal.h (ATTRIBUTE_NONNULL): Define. --- src/internal.h |8 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/src/internal.h b/src/internal.h index 936cd03..8fa579c 100644 --- a/src/internal.h +++ b/src/internal.h @@ -116,6 +116,14 @@ #endif #endif +#ifndef ATTRIBUTE_NONNULL +# if __GNUC_PREREQ (3, 3) +# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) +# else +# define ATTRIBUTE_NONNULL(m) +# endif +#endif + #else #ifndef ATTRIBUTE_UNUSED #define ATTRIBUTE_UNUSED -- 1.6.4.2.395.ge3d52 From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 12:22:14 +0200 Subject: [PATCH 2/2] remote_internal.c: appease clang * src/remote_internal.c (remoteNetworkOpen): Mark conn parameter as non-NULL. Remove now-unnecessary conn == NULL test. (remoteDevMonOpen): Likewise. --- src/remote_internal.c | 10 -- 1 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/remote_internal.c b/src/remote_internal.c index ea50c11..fe36ddd 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3281,13 +3281,12 @@ done: static virDrvOpenStatus remoteNetworkOpen (virConnectPtr conn, virConnectAuthPtr auth, - int flags) + int flags) ATTRIBUTE_NONNULL (1) { if (inside_daemon) return VIR_DRV_OPEN_DECLINED; -if (conn -conn-driver +if (conn-driver STREQ (conn-driver-name, remote)) { struct private_data *priv; @@ -5130,13 +5129,12 @@ done: static virDrvOpenStatus remoteDevMonOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED) + int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1) { if (inside_daemon) return VIR_DRV_OPEN_DECLINED; -if (conn -conn-driver +if (conn-driver STREQ (conn-driver-name, remote)) { struct private_data *priv = conn-privateData; /* If we're here, the remote driver is already ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure
On Wed, Sep 02, 2009 at 10:05:03AM +0200, Jim Meyering wrote: From 7f453c68bc709d542e4c40a388c92c7969ad0a3a Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 09:58:50 +0200 Subject: [PATCH 3/4] lxc: avoid NULL dereference when we find no mount point * src/lxc_container.c (lxcContainerUnmountOldFS): Don't pass a NULL pointer to qsort. --- src/lxc_container.c |5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lxc_container.c b/src/lxc_container.c index 950dd50..2073864 100644 --- a/src/lxc_container.c +++ b/src/lxc_container.c @@ -546,8 +546,9 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt); -qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); +if (mounts) +qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort); for (i = 0 ; i nmounts ; i++) { VIR_DEBUG(Umount %s, mounts[i]); This would is impossible to hit, since you must at least have a /proc filesystem if we've got this far, but doesn't hurt to check anyway :-) ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure
Daniel P. Berrange wrote: On Wed, Sep 02, 2009 at 10:05:03AM +0200, Jim Meyering wrote: From 7f453c68bc709d542e4c40a388c92c7969ad0a3a Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Wed, 2 Sep 2009 09:58:50 +0200 Subject: [PATCH 3/4] lxc: avoid NULL dereference when we find no mount point * src/lxc_container.c (lxcContainerUnmountOldFS): Don't pass a NULL pointer to qsort. --- src/lxc_container.c |5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lxc_container.c b/src/lxc_container.c index 950dd50..2073864 100644 --- a/src/lxc_container.c +++ b/src/lxc_container.c @@ -546,8 +546,9 @@ static int lxcContainerUnmountOldFS(void) } endmntent(procmnt); -qsort(mounts, nmounts, sizeof(mounts[0]), - lxcContainerChildMountSort); +if (mounts) +qsort(mounts, nmounts, sizeof(mounts[0]), + lxcContainerChildMountSort); for (i = 0 ; i nmounts ; i++) { VIR_DEBUG(Umount %s, mounts[i]); This would is impossible to hit, since you must at least have a /proc filesystem if we've got this far, but doesn't hurt to check anyway :-) It can be triggered when the first getmntent call fails. I'll revise the log to mention that. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Don't blindly reorder disk drives
On Mon, Aug 17, 2009 at 03:34:38PM +0100, Daniel P. Berrange wrote: Calling qsort() on the disks array causes disk to be unneccessarily re-ordered, potentially breaking the ability to boot if the boot disk gets moved later in the list. The new algorithm will insert a new disk as far to the end of the list as possible, while being ordered correctly wrt other disks on the same bus. * src/domain_conf.c, src/domain_conf.h: Remove disk sorting routines. Add API to insert a disk into existing list at the optimal position, without resorting disks * src/libvirt_private.syms: Export virDomainDiskInsert * src/xend_internal.c, src/xm_internal.c: Remove calls to qsort, use virDomainDiskInsert instead. * src/qemu_driver.c: Remove calls to qsort, use virDoaminDiskInsert instead. Fix reordering bugs when hotunplugging disks and networks. Fix memory leak in disk/net unplug ACK, an important patch, we really should not drop this ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] Support for CPUID masking
2009/9/2 Jiri Denemark jdene...@redhat.com: Hi, We need to provide support for CPU ID masking. Xen and VMware ESX are examples of current hypervisors which support such masking. [...] domain type='xen' id='42' ... features pae/ acpi/ apic/ cpuid mask level='1' register='ebx' :::1010:::: /mask mask level='1' register='ecx' ::::::xx1x: /mask mask level='1' register='edx' xxx1::::::: /mask mask level='8001' register='ecx' :::::::xx1x /mask mask level='8008' register='ecx' ::::::xx00:1001 /mask /cpuid /features ... /domain I like the proposed mapping for the domain XML, because it's an 1:1 mapping of what VMware uses in the VI API [1] and the VMX config. Beside that VMware has two more possible values for the CPUID bits: H and R. Both are used to define how to handle/interpret those bits in the context of VMotion (migration). For example the domain XML snippet above maps to this VMX snippet: cpuid.1.ebx = 1010 cpuid.1.ecx = XX1X cpuid.1.edx = XXX1 cpuid.8001.ecx = XX1X cpuid.8008.ecx = XX001001 Matthias [1] http://www.vmware.com/support/developer/vc-sdk/visdk400pubs/ReferenceGuide/vim.host.CpuIdInfo.html -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: OpenVZ : The restriction of domain name should be addressed
On Fri, Aug 21, 2009 at 11:08:05AM +0900, Yuji NISHIDA wrote: 2009/7/24 Daniel P. Berrange berra...@redhat.com We should make use of this --name parameter then - I guess it didn't exist when we first wrote the driver. It is useful to users to have separate ID vs Name parameters - and in fact the current reusing of 'ID' for the name, causes a little confusion in virsh's lookup routines because it can't tell whether the parameter its given is a name or an ID, since they are identical. There is still a question of how to specify both a name and CTID in XML description. By default, CTID can be obtained as openvzGimmeFirstUnusedCTID(), but actually I think that there exists a number of persons interested in giving CTIDs manually. Well, can domain id='' be used for CTID remaining name for alphabetical domain name? I worte a small patch and tried with following XML setting. ### Patch ### --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -130,9 +130,6 @@ ADD_ARG_LIT(VZCTL); ADD_ARG_LIT(--quiet); ADD_ARG_LIT(create); -ADD_ARG_LIT(vmdef-id); - -ADD_ARG_LIT(--name); ADD_ARG_LIT(vmdef-name); ### XML ### domain id='100' nameabc/name I found the type of id was identified as number( obj-type == XPATH_NUMBER ) in virXPathLongBase, but it is clearly string before converted. I think correct path is to go in the first if context( obj-type == XPATH_STRING ) and run strtol. the id of domain must be a number, that's a requirement of libvirt so when parsing we expect a number value. Then I tried with following XML. ### XML ### domain id=100 nameabc/name I got following error. AttValue: or ' expected yes, that's not XML ! you need ' or around attributes, normal too. I'm not sure from which function should return this result. I'm not sure what you're trying to do but as is the behaviour of libvirt looks fine from your report. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] UML driver struct refers to public API functions
On Mon, Aug 24, 2009 at 02:13:07PM +0100, Daniel P. Berrange wrote: On Sat, Aug 22, 2009 at 10:04:30PM +0200, Matthias Bolte wrote: Hi, The commit Generic shared impls of all NUMA apis (b0b968efd56f6c66bfa23eebbecd491ea993f99b) changed the UML driver struct to use the shared NUMA API. But now the UML driver struct refers to the public API functions: virNodeGetCellsFreeMemory virNodeGetFreeMemory instead of the shared NUMA API functions nodeGetCellsFreeMemory nodeGetFreeMemory This results in an infinite recursion, if someone's going to call virNodeGetCellsFreeMemory with an UML connection. Opps, that's a bit of a nasty bug. Clearly need to add these APis to the libvirt-TCK tests Apparently the bug seems fixed in git: static virDriver umlDriver = { ... NULL, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ nodeGetFreeMemory, /* getFreeMemory */ NULL, /* domainEventRegister */ so issue seems solved now. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] UML driver struct refers to public API functions
2009/9/2 Daniel Veillard veill...@redhat.com: On Mon, Aug 24, 2009 at 02:13:07PM +0100, Daniel P. Berrange wrote: On Sat, Aug 22, 2009 at 10:04:30PM +0200, Matthias Bolte wrote: Hi, The commit Generic shared impls of all NUMA apis (b0b968efd56f6c66bfa23eebbecd491ea993f99b) changed the UML driver struct to use the shared NUMA API. But now the UML driver struct refers to the public API functions: virNodeGetCellsFreeMemory virNodeGetFreeMemory instead of the shared NUMA API functions nodeGetCellsFreeMemory nodeGetFreeMemory This results in an infinite recursion, if someone's going to call virNodeGetCellsFreeMemory with an UML connection. Opps, that's a bit of a nasty bug. Clearly need to add these APis to the libvirt-TCK tests Apparently the bug seems fixed in git: static virDriver umlDriver = { ... NULL, /* domainMemoryPeek */ nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */ nodeGetFreeMemory, /* getFreeMemory */ NULL, /* domainEventRegister */ so issue seems solved now. Daniel Yes, Daniel P. Berrange fixed it 6 days ago in commit 83af0508007d787e74a57a1fca290f632a58dda7, but he didn't mention it on the mailing list. Matthias -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] RPM spec file updated with glusterfs dependency
On Tue, Sep 01, 2009 at 11:51:22AM +0100, Daniel P. Berrange wrote: On Sat, Aug 29, 2009 at 12:20:11AM +0200, Gerrit Slomma wrote: On Tue, Jul 07, 2009 at 05:47:56AM -0700, Harshavardhana wrote: Add new dependency for glusterfs rpm. [...] +# For glusterfs +Requires: glusterfs-client = 2.0.2 %endif why 2.0.2 ? is taht a hard requirement ? In Fedora/Rawhide we have only 2.0.1 at the moment, Tested libvirt-0.7.0 today. And this requirement really breaks installation of builds on RHEL/CentOS/ScientificLinux. The newest available built for glusterfs is those systems is glusterfs-client-1.3.8 from freshrpms. Even --without-storage-fs does not help a bit, must be pulled in implicitly? No, its an explicitly RPM dependancy rpm -pq --requires libvirt-0.7.0-1.x86_64.rpm (...) glusterfs-client = 2.0.1 (...) Is this patched in upcoming 0.7.1 or how could i go around this issue? We should hjust make this dependancy conditional onn fedora = 11, since no earlier Fedora or RHEL has this available. I see this was fixed in git, cool, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Support for getting/setting number of cpus in VirtualBox
On Fri, Aug 07, 2009 at 04:08:13PM +0200, Pritesh Kothari wrote: Hi All, I have just added support for getting/setting number of cpus in VirtualBox. The patch for the same is include below. Okay, applied and commited, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] VMware ESX: Allow ethernet address type 'vpx'
On Sat, Aug 08, 2009 at 11:56:10PM +0200, Matthias Bolte wrote: The VMX entry ethernet0.addressType may be set to 'vpx' beside 'static' and 'generated'. 'vpx' indicates that the MAC address was generated by a vCenter. The attached patch adds 'vpx' to the valid values for ethernet0.addressType. Okay, I finally commited this patch :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] VMware ESX: Don't warn if a known query parameter should be ignored
On Sat, Aug 08, 2009 at 11:57:25PM +0200, Matthias Bolte wrote: esxUtil_ParseQuery() warns if a known query parameter should be ignored due to the corresponding char/int pointer being NULL, instead of silently ignoring it. The attached patch changes the if/else structure to fix this. Okay, makes sense, applied too :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] rebased multipath patch
Here's the multipath patch, rebased against the current head. Dave From 52cb0ff30dd5a52c5a246f3a2022f933cd2a0eab Mon Sep 17 00:00:00 2001 From: David Allan dal...@redhat.com Date: Tue, 21 Jul 2009 12:43:01 -0400 Subject: [PATCH] Add a simple pool type for multipath devices This pool type contains volumes for the multipath devices that are present on the host. It does not (yet) support any sort of multipath configuration, so that aspect of system administration must still be done at host build time. --- configure.in| 24 +++ po/POTFILES.in |1 + src/Makefile.am |9 + src/storage_backend.c | 82 ++ src/storage_backend.h |5 +- src/storage_backend_mpath.c | 352 +++ src/storage_backend_mpath.h | 31 src/storage_conf.c |7 +- src/storage_conf.h |1 + 9 files changed, 510 insertions(+), 2 deletions(-) create mode 100644 src/storage_backend_mpath.c create mode 100644 src/storage_backend_mpath.h diff --git a/configure.in b/configure.in index d28c44a..7c93ab7 100644 --- a/configure.in +++ b/configure.in @@ -1046,6 +1046,8 @@ AC_ARG_WITH([storage-iscsi], [ --with-storage-iscsiwith iSCSI backend for the storage driver (on)],[],[with_storage_iscsi=check]) AC_ARG_WITH([storage-scsi], [ --with-storage-scsi with SCSI backend for the storage driver (on)],[],[with_storage_scsi=check]) +AC_ARG_WITH([storage-mpath], +[ --with-storage-mpathwith mpath backend for the storage driver (on)],[],[with_storage_mpath=check]) AC_ARG_WITH([storage-disk], [ --with-storage-disk with GPartd Disk backend for the storage driver (on)],[],[with_storage_disk=check]) @@ -1056,6 +1058,7 @@ if test $with_libvirtd = no; then with_storage_lvm=no with_storage_iscsi=no with_storage_scsi=no + with_storage_mpath=no with_storage_disk=no fi if test $with_storage_dir = yes ; then @@ -1177,6 +1180,26 @@ if test $with_storage_scsi = check; then fi AM_CONDITIONAL([WITH_STORAGE_SCSI], [test $with_storage_scsi = yes]) +if test $with_storage_mpath = check; then + with_storage_mpath=yes + + AC_DEFINE_UNQUOTED([WITH_STORAGE_MPATH], 1, + [whether mpath backend for storage driver is enabled]) +fi +AM_CONDITIONAL([WITH_STORAGE_MPATH], [test $with_storage_mpath = yes]) + +if test $with_storage_mpath = yes; then + DEVMAPPER_REQUIRED=0.0 + DEVMAPPER_CFLAGS= + DEVMAPPER_LIBS= + PKG_CHECK_MODULES(DEVMAPPER, devmapper = $DEVMAPPER_REQUIRED, +[], [ +AC_MSG_ERROR( +[You must install device-mapper-devel = $DEVMAPPER_REQUIRED to compile libvirt]) +]) +fi +AC_SUBST([DEVMAPPER_CFLAGS]) +AC_SUBST([DEVMAPPER_LIBS]) LIBPARTED_CFLAGS= LIBPARTED_LIBS= @@ -1677,6 +1700,7 @@ AC_MSG_NOTICE([ NetFS: $with_storage_fs]) AC_MSG_NOTICE([ LVM: $with_storage_lvm]) AC_MSG_NOTICE([ iSCSI: $with_storage_iscsi]) AC_MSG_NOTICE([SCSI: $with_storage_scsi]) +AC_MSG_NOTICE([ mpath: $with_storage_mpath]) AC_MSG_NOTICE([Disk: $with_storage_disk]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Security Drivers]) diff --git a/po/POTFILES.in b/po/POTFILES.in index 0a53dab..1586368 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -38,6 +38,7 @@ src/storage_backend_fs.c src/storage_backend_iscsi.c src/storage_backend_logical.c src/storage_backend_scsi.c +src/storage_backend_mpath.c src/storage_conf.c src/storage_driver.c src/storage_encryption_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index bedeb84..cf3420b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -199,6 +199,9 @@ STORAGE_DRIVER_ISCSI_SOURCES = \ STORAGE_DRIVER_SCSI_SOURCES = \ storage_backend_scsi.h storage_backend_scsi.c +STORAGE_DRIVER_MPATH_SOURCES = \ + storage_backend_mpath.h storage_backend_mpath.c + STORAGE_DRIVER_DISK_SOURCES = \ storage_backend_disk.h storage_backend_disk.c @@ -478,6 +481,10 @@ if WITH_STORAGE_SCSI libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SCSI_SOURCES) endif +if WITH_STORAGE_MPATH +libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES) +endif + if WITH_STORAGE_DISK libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_DISK_SOURCES) endif @@ -539,6 +546,7 @@ EXTRA_DIST += \ $(STORAGE_DRIVER_LVM_SOURCES) \ $(STORAGE_DRIVER_ISCSI_SOURCES) \ $(STORAGE_DRIVER_SCSI_SOURCES) \ + $(STORAGE_DRIVER_MPATH_SOURCES) \ $(STORAGE_DRIVER_DISK_SOURCES) \ $(NODE_DEVICE_DRIVER_SOURCES) \ $(NODE_DEVICE_DRIVER_HAL_SOURCES) \ @@ -607,6 +615,7 @@
Re: [libvirt] [RFC] Support for CPUID masking
Jiri Denemark wrote: Hi, We need to provide support for CPU ID masking. Xen and VMware ESX are examples of current hypervisors which support such masking. My proposal is to define new 'cpuid' feature advertised in guest capabilities: ... domain type='xen' id='42' ... features pae/ acpi/ apic/ cpuid mask level='1' register='ebx' :::1010:::: /mask ... What are your opinions about this? I think it's too low-level, and the structure is x86-specific. QEMU and KVM compute their CPUID response based on arguments to the -cpu argument, e.g.: -cpu core2duo,model=23,+ssse3,+lahf_lm I think a similar structure makes more sense for libvirt, where the configuration generally avoids big blocks of binary data, and the XML format should suit other architectures as well. -jim -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] Support for CPUID masking
On Wed, Sep 02, 2009 at 11:59:39AM -0400, Jim Paris wrote: Jiri Denemark wrote: Hi, We need to provide support for CPU ID masking. Xen and VMware ESX are examples of current hypervisors which support such masking. My proposal is to define new 'cpuid' feature advertised in guest capabilities: ... domain type='xen' id='42' ... features pae/ acpi/ apic/ cpuid mask level='1' register='ebx' :::1010:::: /mask ... What are your opinions about this? I think it's too low-level, and the structure is x86-specific. QEMU and KVM compute their CPUID response based on arguments to the -cpu argument, e.g.: -cpu core2duo,model=23,+ssse3,+lahf_lm I think a similar structure makes more sense for libvirt, where the configuration generally avoids big blocks of binary data, and the XML format should suit other architectures as well. I'm going back forth on this too. We essentially have 3 options - Named CPU + flags/features - CPUID masks - Allow either If we do either of the first two, we have to translate between the two formats for one or more of the hypervisors. For the last one we are just punting the problem off to applications. If we choose CPUID, and made QEMU driver convert to named CPU + flags we'd be stuck for non-x86 as you say. If we chose named CPU + flags, and made VMWare/Xen convert to raw CPUID we'd potentially loose information if user had defined a config with a raw CPUID mask outside context of libvirt. The other thing to remember is that CPUID also encodes sockets/cores/ threads topology data, and it'd be very desirable to expose that in a sensible fashion (ie not a bitmask). On balance i'm currently leaning to named CPU + flags + expliciti topology data because although its harder to implement for Xen/VMWare I think its much nicer to applications users. We might loose a tiny bit of data in the CPU - named/flags conversion for Xen/VMWare but I reckon we can get it good enough that most people won't really care about that. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] virsh vcpuinfo always returns CPU=0
Hi there, I wonder what does the CPU entry stands for in the output of vcpuinfo. I guess it is the CPU utilization rate, right? For both Ubuntu 9.04 (virsh version 0.6.1) and RHEL 5.4 (version 0.6.3), I always get the returned CPU value to be 0, no matter how busy the CPU can be and how many VCPU is used. For example, onnecting to uri: qemu:///system VCPU: 0 CPU:0 State: running CPU Affinity: VCPU: 1 CPU:0 State: running CPU Affinity: Is this a problem and how do I fix it? Thank you very much. -- Shi Jin, PhD -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh vcpuinfo always returns CPU=0
On Wed, Sep 02, 2009 at 09:37:15AM -0700, Shi Jin wrote: Hi there, I wonder what does the CPU entry stands for in the output of vcpuinfo. I guess it is the CPU utilization rate, right? It is the most recent physical CPU in which the vCPU ran. For both Ubuntu 9.04 (virsh version 0.6.1) and RHEL 5.4 (version 0.6.3), I always get the returned CPU value to be 0, no matter how busy the CPU can be and how many VCPU is used. For example, onnecting to uri: qemu:///system VCPU: 0 CPU:0 State: running CPU Affinity: VCPU: 1 CPU:0 State: running CPU Affinity: Is this a problem and how do I fix it? Upgrade to newer libvirt with this patch in http://libvirt.org/git/?p=libvirt.git;a=commit;h=c4a04dc0240589031ba1042f446095fb69222040 Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix several memory leaks
Hi Chris, On Tue, Sep 1, 2009 at 4:45 AM, Chris Lalancetteclala...@redhat.com wrote: Ryota Ozaki wrote: diff --git a/qemud/qemud.c b/qemud/qemud.c index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore: /* Possibly need to create another receive buffer */ if ((client-nrequests max_client_requests - VIR_ALLOC(client-rx) 0)) { + !client-rx VIR_ALLOC(client-rx) 0)) { qemudDispatchClientFailure(client); } else { if (client-rx) Hm, I'm not super familiar with this section of code, but this doesn't seem to be right. How can client-rx ever be NULL here? We dereference client-rx-bufferOffset very early on in the function, so we would have crashed before we got here. What am I missing? No, you are probably right. As Daniel also says, the fix seems wrong. I actually used valgrind to find memory leaks and valgrind pointed out here. However, now the leak cannot be reproduced anymore, so probably I missed something... I'll drop this fix in the next patch. diff --git a/src/domain_conf.c b/src/domain_conf.c index 1d2cc7c..4b64219 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn, cleanup: if (fd != -1) close(fd); + VIR_FREE(configFile); return ret; } This one looks good. diff --git a/src/network_conf.c b/src/network_conf.c index bb649a4..58a4f32 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn, { char *configFile = NULL; char *autostartLink = NULL; + int ret = -1; if ((configFile = virNetworkConfigFile(conn, configDir, net-def-name)) == NULL) goto error; @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn, goto error; } - return 0; + ret = 0; error: VIR_FREE(configFile); VIR_FREE(autostartLink); - return -1; + return ret; } This one looks good. char *virNetworkConfigFile(virConnectPtr conn, diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f5edd..32d6a48 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, virDomainNetDefPtr net, int qemuCmdFlags) { - char *brname; + char *brname = NULL; int err; int tapfd = -1; int vnet_hdr = 0; @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net-type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = net-data.bridge.brname; + brname = strdup(net-data.bridge.brname); } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _(Network type %d is not supported), net-type); @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (!driver-brctl (err = brInit(driver-brctl))) { virReportSystemError(conn, err, %s, _(cannot initialize bridge support)); - return -1; + goto cleanup; } if (!net-ifname || @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn, VIR_FREE(net-ifname); if (!(net-ifname = strdup(vnet%d))) { virReportOOMError(conn); - return -1; + goto cleanup; } /* avoid exposing vnet%d in dumpxml or error outputs */ template_ifname = 1; @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn, } if (template_ifname) VIR_FREE(net-ifname); - return -1; + tapfd = -1; } +cleanup: + VIR_FREE(brname); + return tapfd; } This generally looks OK. diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, vol-type = VIR_STORAGE_VOL_FILE; vol-target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol-target.path) + VIR_FREE(vol-target.path); Again, how can this ever be the case? The first time through the loop vol is definitely NULL, so there's no possibility vol-target.path contains any allocated memory. At the end of this loop, we assign the whole vol object to a slot in the pool-volumes array, set vol to NULL, and then subsequent iterations get a new vol object allocated. So how can this ever be true? I'm wrong again... I was too easy to add the fix along with the below fix. I'll drop it as well. if (virAsprintf(vol-target.path, %s/%s, pool-def-target.path,
Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn
Does __attribute__((__nonnull__())) really cover the case we're concerned about here? No, not at all. Good catch. Paolo -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Support configuration of huge pages in guests
Daniel P. Berrange wrote: * configure.in: Add check for mntent.h * qemud/libvirtd_qemu.aug, qemud/test_libvirtd_qemu.aug, src/qemu.conf Add 'hugetlbfs_mount' config parameter * src/qemu_conf.c, src/qemu_conf.h: Check for -mem-path flag in QEMU, and pass it when hugepages are requested. Load hugetlbfs_mount config parameter, search for mount if not given. * src/qemu_driver.c: Free hugetlbfs_mount/path parameter in driver shutdown. Create directory for QEMU hugepage usage, chowning if required. * docs/formatdomain.html.in: Document memoryBacking/hugepages elements * docs/schemas/domain.rng: Add memoryBacking/hugepages elements to schema * src/util.c, src/util.h, src/libvirt_private.syms: Add virFileFindMountPoint helper API * tests/qemuhelptest.c: Add -mem-path constants * tests/qemuxml2argvtest.c, tests/qemuxml2xmltest.c: Add tests for hugepage handling * tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml, tests/qemuxml2argvdata/qemuxml2argv-hugepages.args: Data files for hugepage tests --- configure.in |2 +- docs/formatdomain.html |8 +++- docs/formatdomain.html.in |8 +++ docs/schemas/domain.rng|9 qemud/libvirtd_qemu.aug|1 + qemud/test_libvirtd_qemu.aug |4 ++ src/domain_conf.c | 10 - src/domain_conf.h |1 + src/libvirt_private.syms |1 + src/qemu.conf | 13 + src/qemu_conf.c| 49 src/qemu_conf.h|3 + src/qemu_driver.c | 34 ++ src/util.c | 37 ++- src/util.h |4 ++ tests/qemuhelptest.c |6 ++- tests/qemuxml2argvdata/qemuxml2argv-hugepages.args |1 + tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml | 25 ++ tests/qemuxml2argvtest.c |7 ++- tests/qemuxml2xmltest.c|1 + 20 files changed, 217 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml I did a fetch on libvirt.git before reviewing and it appears there is some code motion relative to the version this patch was against. Although AFAICT nothing which appears to result in more than patch bounce. Looks good to me. ACK. -john -- john.coo...@redhat.com -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list