July 2010 - Devel - libvirt List Archives

[libvirt] [PATCH] Fix the ACS checking in the PCI code.
by Chris Lalancette 02 Aug '10

02 Aug '10

When trying to assign a PCI device to a guest, we have to check that all bridges upstream of that device support ACS. That means that we have to find the parent bridge of the current device, check for ACS, then find the parent bridge of that device, check for ACS, etc. As it currently stands, the code to do this iterates through all PCI devices on the system, looking for a device that has a range of busses that included the current device's bus. That check is not restrictive enough, though. Depending on how we iterated through the list of PCI devices, we could first find the *topmost* bridge in the system; since it necessarily had a range of busses including the current device's bus, we would only ever check the topmost bridge, and not check any of the intermediate bridges. Note that this also caused a fairly serious bug in the secondary bus reset code, where we could erroneously find and reset the topmost bus instead of the inner bus. This patch changes pciGetParentDevice() so that it first checks if a bridge device's secondary bus exactly matches the bus of the device we are looking for. If it does, we've found the correct parent bridge and we are done. If it does not, then we check to see if this bridge device's busses *include* the bus of the device we care about. If so, we mark this bridge device as best, and go on. If we later find another bridge device whose busses include this device, but is more restrictive, then we free up the previous best and mark the new one as best. This algorithm ensures that in the normal case we find the direct parent, but in the case that the parent bridge secondary bus is not exactly the same as the device, we still find the correct bridge. This patch was tested by me on a 4-port NIC with a bridge without ACS (where assignment failed), a 4-port NIC with a bridge with ACS (where assignment succeeded), and a 2-port NIC with no bridges (where assignment succeeded). Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- src/util/pci.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 66 insertions(+), 12 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 26d55b8..f2890bd 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -283,6 +283,7 @@ pciIterDevices(pciIterPredicate predicate, DIR *dir; struct dirent *entry; int ret = 0; + int rc; *matched = NULL; @@ -322,11 +323,20 @@ pciIterDevices(pciIterPredicate predicate, break; } - if (predicate(dev, check, data)) { + rc = predicate(dev, check, data); + if (rc < 0) { + /* the predicate returned an error, bail */ + pciFreeDevice(check); + ret = -1; + break; + } + else if (rc == 1) { VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name); *matched = check; + ret = 1; break; } + pciFreeDevice(check); } closedir(dir); @@ -510,10 +520,11 @@ pciBusContainsActiveDevices(pciDevice *dev, /* Is @check the parent of @dev ? */ static int -pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) +pciIsParent(pciDevice *dev, pciDevice *check, void *data) { uint16_t device_class; uint8_t header_type, secondary, subordinate; + pciDevice **best = data; if (dev->domain != check->domain) return 0; @@ -533,16 +544,54 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) VIR_DEBUG("%s %s: found parent device %s", dev->id, dev->name, check->name); - /* No, it's superman! */ - return (dev->bus >= secondary && dev->bus <= subordinate); + /* if the secondary bus exactly equals the device's bus, then we found + * the direct parent. No further work is necessary + */ + if (dev->bus == secondary) + return 1; + + /* otherwise, SRIOV allows VFs to be on different busses then their PFs. + * In this case, what we need to do is look for the "best" match; i.e. + * the most restrictive match that still satisfies all of the conditions. + */ + if (dev->bus > secondary && dev->bus <= subordinate) { + if (*best == NULL) { + *best = pciGetDevice(check->domain, check->bus, check->slot, + check->function); + if (*best == NULL) + return -1; + } + else { + /* OK, we had already recorded a previous "best" match for the + * parent. See if the current device is more restrictive than the + * best, and if so, make it the new best + */ + if (secondary > pciRead8(*best, PCI_SECONDARY_BUS)) { + pciFreeDevice(*best); + *best = pciGetDevice(check->domain, check->bus, check->slot, + check->function); + if (*best == NULL) + return -1; + } + } + } + + return 0; } -static pciDevice * -pciGetParentDevice(pciDevice *dev) +static int +pciGetParentDevice(pciDevice *dev, pciDevice **parent) { - pciDevice *parent = NULL; - pciIterDevices(pciIsParent, dev, &parent, NULL); - return parent; + pciDevice *best = NULL; + int ret; + + *parent = NULL; + ret = pciIterDevices(pciIsParent, dev, parent, &best); + if (ret == 1) + pciFreeDevice(best); + else if (ret == 0) + *parent = best; + return ret; } /* Secondary Bus Reset is our sledgehammer - it resets all @@ -570,7 +619,8 @@ pciTrySecondaryBusReset(pciDevice *dev, } /* Find the parent bus */ - parent = pciGetParentDevice(dev); + if (pciGetParentDevice(dev, &parent) < 0) + return -1; if (!parent) { pciReportError(VIR_ERR_NO_SUPPORT, _("Failed to find parent device for %s"), @@ -1377,7 +1427,8 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) { pciDevice *parent; - parent = pciGetParentDevice(dev); + if (pciGetParentDevice(dev, &parent) < 0) + return -1; if (!parent) { /* if we have no parent, and this is the root bus, ACS doesn't come * into play since devices on the root bus can't P2P without going @@ -1400,6 +1451,7 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) do { pciDevice *tmp; int acs; + int ret; acs = pciDeviceDownstreamLacksACS(parent); @@ -1412,8 +1464,10 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) } tmp = parent; - parent = pciGetParentDevice(parent); + ret = pciGetParentDevice(parent, &parent); pciFreeDevice(tmp); + if (ret < 0) + return -1; } while (parent); return 0; -- 1.7.1.1

3 3

[libvirt] [PATCH] Free up memballoon def.
by Chris Lalancette 02 Aug '10

02 Aug '10

Forgetting to do this was causing a memory leak. Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- src/conf/domain_conf.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ca4bc6e..1ddea0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -768,6 +768,8 @@ void virDomainDefFree(virDomainDefPtr def) virDomainWatchdogDefFree(def->watchdog); + virDomainMemballoonDefFree(def->memballoon); + virSecurityLabelDefFree(def); virCPUDefFree(def->cpu); -- 1.7.2

2 2

[libvirt] [PATCH] Don't put a semicolon on the end of a VIR_ENUM_IMPL.
by Chris Lalancette 02 Aug '10

02 Aug '10

Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 04c417e..ca4bc6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -99,7 +99,7 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "none", "pci", "drive", - "virtio-serial"); + "virtio-serial") VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST, "block", -- 1.7.2

2 2

[libvirt] [PATCH] Fix a bogus warning when parsing <hostdev>
by Chris Lalancette 02 Aug '10

02 Aug '10

When parsing hostdev, the following message would be emitted: 10:17:19.052: error : virDomainHostdevDefParseXML:3748 : internal error unknown node alias However, alias is appropriately parsed in virDomainDeviceInfoParseXML anyway. Disable the error message in the initial XML parsing loop. Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- src/conf/domain_conf.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8721dd1..04c417e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3723,6 +3723,9 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "address")) { + /* address is parsed as part of virDomainDeviceInfoParseXML */ + } else if (xmlStrEqual(cur->name, BAD_CAST "alias")) { + /* alias is parsed as part of virDomainDeviceInfoParseXML */ } else { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown node %s"), cur->name); -- 1.7.2

2 2

[libvirt] default storage pool will not start
by Albert Hopkins 01 Aug '10

01 Aug '10

Long ago I had a VM that used a qcow image that was in the default directory-based storage pool. Since then the VM and the image have been deleted. But now libvirt will not start without the image file: Traceback (most recent call last): File "/usr/share/virt-manager/virtManager/host.py", line 586, in start_pool pool.start() File "/usr/share/virt-manager/virtManager/storagepool.py", line 67, in start self.pool.create(0) File "/usr/lib64/python2.6/site-packages/libvirt.py", line 1039, in create if ret == -1: raise libvirtError ('virStoragePoolCreate() failed', pool=self) libvirtError: cannot open volume '/var/lib/libvirt/images/gstable.qcow2': No such file or directory If I simply "touch" the file, then the pool starts. But I don't want it around. No other vm's reference this file. I've tried deleting and re-creating the pool. I've tried removing all the files in the pool, removing the directory, mkdir'ing the directory and then re-creating the pool, but it still will not start without that file. I've grepped /etc/libvirt for the existence of that filename but it doesn't exist. For the life of me I can't figure out where libvirt is getting this information from and why it thinks it needs this file.

2 3

[libvirt] OpenBSD crash after "shutdown vm"
by Armando 31 Jul '10

31 Jul '10

Good evening all, firstly I'd like to thank you for your wonderful job: it works nicely and simplifies a lot my life :) after saying that, I'd like to expose you a "bug" (not sure if I can call it this way): 1. scenario: as host I've configured a Ubuntu 10.04 with KVM + libvirt + LVM; I have also configured 4 virtual machines, three of wich are OpenBSDs and the other one is Linux (Fedora 13). 2. issue: when I issue a shutdown to a OpenBSD guest using "virsh shutdown vm" what happens is that the guest machine crashes. To be more precise it seems a kernel panic becuase the vnc session through KVM still works as the machine is running but every digit I issue does not appear on the server shell. Also the ssh sessions die; After having a chat in the #libvirt channel I tried doing this both with acpibtn enabled (on OpenBSD is the default behaviour) and also with acpibtn disabled (using the command "boot -c" and then "disable acpi" at boot time in the OpenBSD server). In both cases the server crashes At this point I have found an alternative way to automatically shut down the machine (using ssh) but it would be nice to understand way this happens. Is this a well-known issue? Is there a way to deeply debug it? Thanks a lot! 0x412E

1 0

[libvirt] [PATCH] Update ID after stopping a domain
by Matthias Bolte 30 Jul '10

30 Jul '10

--- src/esx/esx_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 2 ++ src/vbox/vbox_tmpl.c | 1 + src/xenapi/xenapi_driver.c | 1 + 5 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 964a3a5..913420c 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1620,6 +1620,7 @@ esxDomainDestroy(virDomainPtr domain) goto cleanup; } + domain->id = -1; result = 0; cleanup: diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 98381fb..c46f3a7 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -531,6 +531,7 @@ static int openvzDomainShutdown(virDomainPtr dom) { vm->def->id = -1; vm->state = VIR_DOMAIN_SHUTOFF; + dom->id = -1; ret = 0; cleanup: diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index e4afc5a..7143933 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3688,6 +3688,8 @@ phypDomainDestroy(virDomainPtr dom) if (phypUUIDTable_RemLpar(dom->conn, dom->id) == -1) goto err; + dom->id = -1; + VIR_FREE(cmd); VIR_FREE(ret); return 0; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 0a91c7f..31fec67 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1587,6 +1587,7 @@ static int vboxDomainDestroy(virDomainPtr dom) { } #endif VBOX_RELEASE(console); + dom->id = -1; ret = 0; } data->vboxSession->vtbl->Close(data->vboxSession); diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 2262cef..fb3c91d 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -830,6 +830,7 @@ xenapiDomainDestroy (virDomainPtr dom) return -1; } xen_vm_set_free(vms); + dom->id = -1; return 0; } if (vms) xen_vm_set_free(vms); -- 1.7.0.4

2 2

[libvirt] [PATCH] openvzDomainCreateWithFlags: set domain id to the correct value
by Jean-Baptiste Rouault 30 Jul '10

30 Jul '10

I'm not sure if it's ok or not to modify dom->id as I did here. Regards, Jean-Baptiste

2 2

[libvirt] [PATCH] storage: kill dead stores
by Eric Blake 30 Jul '10

30 Jul '10

Found by clang. Clang complained that virStorageBackendProbeTarget could dereference NULL if backingStoreFormat was NULL, but since all callers passed a valid pointer, I added attributes instead of null checks. * src/storage/storage_backend.c (virStorageBackendQEMUImgBackingFormat): Kill dead store. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. Skip null checks, by adding attributes. --- Thankfully, the null dereference scenario noted by clang was never triggered in the code, which is good since it was introduced as part of fixing a CVE. src/storage/storage_backend.c | 3 +-- src/storage/storage_backend_fs.c | 34 +++++++++++++++------------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d989743..1fe7ba6 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -581,7 +581,6 @@ static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) int newstdout = -1; char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 1024*8 }; - int len; char *start; char *end; char *tmp; @@ -591,7 +590,7 @@ static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0) goto cleanup; - if ((len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help)) < 0) { + if (virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help) < 0) { virReportSystemError(errno, _("Unable to read '%s -h' output"), qemuimg); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0761bd8..b6b8fdd 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -48,7 +48,7 @@ #define VIR_FROM_THIS VIR_FROM_STORAGE -static int +static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virStorageBackendProbeTarget(virStorageVolTargetPtr target, char **backingStore, int *backingStoreFormat, @@ -59,10 +59,8 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, int fd, ret; virStorageFileMetadata meta; - if (backingStore) - *backingStore = NULL; - if (backingStoreFormat) - *backingStoreFormat = VIR_STORAGE_FILE_AUTO; + *backingStore = NULL; + *backingStoreFormat = VIR_STORAGE_FILE_AUTO; if (encryption) *encryption = NULL; @@ -75,7 +73,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, allocation, capacity)) < 0) { close(fd); - return -1; + return ret; } memset(&meta, 0, sizeof(meta)); @@ -95,20 +93,19 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, close(fd); if (meta.backingStore) { - if (backingStore) { - *backingStore = meta.backingStore; - meta.backingStore = NULL; - if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) { - if ((*backingStoreFormat = virStorageFileProbeFormat(*backingStore)) < 0) { - close(fd); - goto cleanup; - } - } else { - *backingStoreFormat = meta.backingStoreFormat; + *backingStore = meta.backingStore; + meta.backingStore = NULL; + if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) { + if ((*backingStoreFormat + = virStorageFileProbeFormat(*backingStore)) < 0) { + close(fd); + goto cleanup; } } else { - VIR_FREE(meta.backingStore); + *backingStoreFormat = meta.backingStoreFormat; } + } else { + VIR_FREE(meta.backingStore); } if (capacity && meta.capacity) @@ -139,8 +136,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, return 0; cleanup: - if (backingStore) - VIR_FREE(*backingStore); + VIR_FREE(*backingStore); return -1; } -- 1.7.2

2 2

[libvirt] [PATCH] network: kill dead store
by Eric Blake 30 Jul '10

30 Jul '10

* src/network/bridge_driver.c (networkDefine): Kill dead store. --- src/network/bridge_driver.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b7ee880..217cdcc 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1490,14 +1490,13 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { virNetworkDefPtr def; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; - int dupNet; networkDriverLock(driver); if (!(def = virNetworkDefParseString(xml))) goto cleanup; - if ((dupNet = virNetworkObjIsDuplicate(&driver->networks, def, 0)) < 0) + if (virNetworkObjIsDuplicate(&driver->networks, def, 0) < 0) goto cleanup; if (virNetworkSetBridgeName(&driver->networks, def, 1)) -- 1.7.2

2 2