[PATCH RESEND 19/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListDel()

[Daniel Henrique Barboza]

This change will allow us to remove PCI devices from a list
without the need of a PCI Device object, which will be need
in the next patch.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 7 ---
 src/util/virpci.c   | 6 +++---
 src/util/virpci.h   | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 4042f874d6..c708791eec 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -1005,11 +1005,11 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr 
mgr,
 if (STRNEQ_NULLABLE(drv_name, actual_drvname) ||
 STRNEQ_NULLABLE(dom_name, actual_domname)) {
 
-virPCIDeviceListDel(pcidevs, pci);
+virPCIDeviceListDel(pcidevs, virPCIDeviceGetAddress(pci));
 continue;
 }
 } else {
-virPCIDeviceListDel(pcidevs, pci);
+virPCIDeviceListDel(pcidevs, virPCIDeviceGetAddress(pci));
 continue;
 }
 
@@ -2522,7 +2522,8 @@ virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr 
hostdev_mgr,
 while (lastGoodPCIIdx >= 0) {
 virPCIDevicePtr actual = virPCIDeviceListGet(pciDevices, i);
 
-virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, actual);
+virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs,
+virPCIDeviceGetAddress(actual));
 
 lastGoodPCIIdx--;
 }
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 332e681c43..76dfd71802 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1336,7 +1336,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
 /* Steal the dev from list inactiveDevs */
 if (inactiveDevs) {
 VIR_DEBUG("Removing PCI device %s from inactive list", dev->name);
-virPCIDeviceListDel(inactiveDevs, dev);
+virPCIDeviceListDel(inactiveDevs, >address);
 }
 
 return 0;
@@ -1750,9 +1750,9 @@ virPCIDeviceListSteal(virPCIDeviceListPtr list,
 
 void
 virPCIDeviceListDel(virPCIDeviceListPtr list,
-virPCIDevicePtr dev)
+virPCIDeviceAddressPtr devAddr)
 {
-virPCIDeviceFree(virPCIDeviceListSteal(list, >address));
+virPCIDeviceFree(virPCIDeviceListSteal(list, devAddr));
 }
 
 int
diff --git a/src/util/virpci.h b/src/util/virpci.h
index e3458a75fa..eb71eb4451 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -165,7 +165,7 @@ virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr 
list,
 virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
int idx);
 void virPCIDeviceListDel(virPCIDeviceListPtr list,
- virPCIDevicePtr dev);
+ virPCIDeviceAddressPtr devAddr);
 virPCIDevicePtr virPCIDeviceListFind(virPCIDeviceListPtr list,
  virPCIDeviceAddressPtr devAddr);
 virPCIDevicePtr
-- 
2.26.2

[PATCH RESEND 16/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFindIndex()

[Daniel Henrique Barboza]

We're going to need a way to remove a PCI Device from a list without having
a valid virPCIDevicePtr, because the device is missing from the host. This
means that virPCIDevicesListDel() must operate with a PCI Device address
instead.

Turns out that virPCIDevicesListDel() and its related functions only use
the virPCIDeviceAddressPtr of the virPCIDevicePtr, so this change is
simple to do and will not cause hassle in all other callers. Let's
start adapting virPCIDeviceListFindIndex() and crawl our way up to
virPCIDevicesListDel().

Signed-off-by: Daniel Henrique Barboza 
---
 src/util/virpci.c | 15 ---
 src/util/virpci.h |  2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 7143380348..1554acffb6 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1745,7 +1745,7 @@ virPCIDevicePtr
 virPCIDeviceListSteal(virPCIDeviceListPtr list,
   virPCIDevicePtr dev)
 {
-return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
dev));
+return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
>address));
 }
 
 void
@@ -1756,16 +1756,17 @@ virPCIDeviceListDel(virPCIDeviceListPtr list,
 }
 
 int
-virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev)
+virPCIDeviceListFindIndex(virPCIDeviceListPtr list,
+  virPCIDeviceAddressPtr devAddr)
 {
 size_t i;
 
 for (i = 0; i < list->count; i++) {
 virPCIDevicePtr other = list->devs[i];
-if (other->address.domain   == dev->address.domain &&
-other->address.bus  == dev->address.bus&&
-other->address.slot == dev->address.slot   &&
-other->address.function == dev->address.function)
+if (other->address.domain   == devAddr->domain &&
+other->address.bus  == devAddr->bus&&
+other->address.slot == devAddr->slot   &&
+other->address.function == devAddr->function)
 return i;
 }
 return -1;
@@ -1798,7 +1799,7 @@ virPCIDeviceListFind(virPCIDeviceListPtr list, 
virPCIDevicePtr dev)
 {
 int idx;
 
-if ((idx = virPCIDeviceListFindIndex(list, dev)) >= 0)
+if ((idx = virPCIDeviceListFindIndex(list, >address)) >= 0)
 return list->devs[idx];
 else
 return NULL;
diff --git a/src/util/virpci.h b/src/util/virpci.h
index a9c597a428..8c6776da21 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -175,7 +175,7 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list,
   unsigned int slot,
   unsigned int function);
 int virPCIDeviceListFindIndex(virPCIDeviceListPtr list,
-  virPCIDevicePtr dev);
+  virPCIDeviceAddressPtr devAddr);
 
 /*
  * Callback that will be invoked once for each file
-- 
2.26.2

[PATCH RESEND 14/20] virhostdev.c: add virHostdevIsPCIDevice() helper

[Daniel Henrique Barboza]

Add a helper to quickly determine if a hostdev is a PCI device,
instead of doing a tedius 'if' check with hostdev mode and
subsys type.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 12 +---
 src/hypervisor/virhostdev.h |  2 ++
 src/libvirt_private.syms|  1 +
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index dbba36193b..402d7be42d 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -347,12 +347,18 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
 }
 
 
+bool
+virHostdevIsPCIDevice(const virDomainHostdevDef *hostdev)
+{
+return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
+}
+
+
 static bool
 virHostdevIsPCINetDevice(const virDomainHostdevDef *hostdev)
 {
-return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
-hostdev->parentnet != NULL;
+return virHostdevIsPCIDevice(hostdev) && hostdev->parentnet != NULL;
 }
 
 
diff --git a/src/hypervisor/virhostdev.h b/src/hypervisor/virhostdev.h
index 811bda40ed..b9407cd837 100644
--- a/src/hypervisor/virhostdev.h
+++ b/src/hypervisor/virhostdev.h
@@ -235,3 +235,5 @@ virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr 
hostdev_mgr,
   const char *dom_name,
   virDomainDiskDefPtr *disks,
   size_t ndisks);
+
+bool virHostdevIsPCIDevice(const virDomainHostdevDef *hostdev);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 36635e6de7..01fc50b687 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1461,6 +1461,7 @@ virCloseCallbacksUnset;
 
 # hypervisor/virhostdev.h
 virHostdevFindUSBDevice;
+virHostdevIsPCIDevice;
 virHostdevManagerGetDefault;
 virHostdevPCINodeDeviceDetach;
 virHostdevPCINodeDeviceReAttach;
-- 
2.26.2

[PATCH RESEND 20/20] virhostdev.c: remove missing PCI devs from hostdev manager

[Daniel Henrique Barboza]

virHostdevReAttachPCIDevices() is called when we want to re-attach
a list of hostdevs back to the host, either on the shutdown path or
via a 'virsh detach-device' call.  This function always count on the
existence of the device in the host to work, but this can lead to
problems. For example, a SR-IOV device can be removed via an admin
"echo 0 > /sys/bus/pci/devices//sriov_numvfs", making the kernel
fire up and eventfd_signal() to the process, asking for the process to
release the device. The result might vary depending on the device driver
and OS/arch, but two possible outcomes are:

1) the hypervisor driver will detach the device from the VM, issuing a
delete event to Libvirt. This can be observed in QEMU;

2) the 'echo 0 > ...' will hang waiting for the device to be unplugged.
This means that the VM process failed/refused to release the hostdev back
to the host, and the hostdev will be detached during VM shutdown.

Today we don't behave well for both cases. We'll fail to remove the PCI device
reference from mgr->activePCIHostdevs and mgr->inactivePCIHostdevs because
we rely on the existence of the PCI device conf file in the sysfs. Attempting
to re-utilize the same device (assuming it is now present back in the host)
can result in an error like this:

$ ./run tools/virsh start vm1-sriov --console
error: Failed to start domain vm1-sriov
error: Requested operation is not valid: PCI device :01:00.2 is in use by 
driver QEMU, domain vm1-sriov

For (1), a VM destroy/start cycle is needed to re-use the VF in the guest.
For (2), the effect is more nefarious, requiring a Libvirtd daemon restart
to use the VF again in any guest.

We can make it a bit better by checking, during virHostdevReAttachPCIDevices(),
if there is any missing PCI device that will be left behind in activePCIHostdevs
and inactivePCIHostdevs lists. Remove any missing device found from both lists,
unconditionally, matching the current state of the host. This change affects
the code path in (1) (processDeviceDeletedEvent into qemuDomainRemoveDevice, all
the way back to qemuHostdevReAttachPCIDevices) and also in (b) (qemuProcessStop
into qemuHostdevReAttachDomainDevices).

NB: Although this patch enables the possibility of 'outside Libvirt' SR-IOV
hotunplug of PCI devices, if the hypervisor and the PCI driver copes with it,
our goal is to mitigate what it is still considered an user oopsie. For all
supported purposes, the admin must remove the SR-IOV VFs from all running 
domains
before removing the VFs from the host.

Resolves:  https://gitlab.com/libvirt/libvirt/-/issues/72
Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 38 +
 1 file changed, 38 insertions(+)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index c708791eec..ed43733e71 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -1077,6 +1077,40 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr 
mgr,
 }
 
 
+static void
+virHostdevDeleteMissingPCIDevices(virHostdevManagerPtr mgr,
+  virDomainHostdevDefPtr *hostdevs,
+  int nhostdevs)
+{
+size_t i;
+
+if (nhostdevs == 0)
+return;
+
+virObjectLock(mgr->activePCIHostdevs);
+virObjectLock(mgr->inactivePCIHostdevs);
+
+for (i = 0; i < nhostdevs; i++) {
+virDomainHostdevDef *hostdev = hostdevs[i];
+virDomainHostdevSubsysPCI *pcisrc = >source.subsys.u.pci;
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (virHostdevGetPCIHostDevice(hostdev, ) != -2)
+continue;
+
+/* The PCI device from 'hostdev' does not exist in the host
+ * anymore. Delete it from both active and inactive lists to
+ * reflect the current host state.
+ */
+virPCIDeviceListDel(mgr->activePCIHostdevs, >addr);
+virPCIDeviceListDel(mgr->inactivePCIHostdevs, >addr);
+}
+
+virObjectUnlock(mgr->activePCIHostdevs);
+virObjectUnlock(mgr->inactivePCIHostdevs);
+}
+
+
 /* @oldStateDir:
  * For upgrade purpose: see virHostdevRestoreNetConfig
  */
@@ -1102,6 +1136,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
 
 virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs,
  hostdevs, nhostdevs, oldStateDir);
+
+/* Handle the case where PCI devices from the host went missing
+ * during the domain lifetime */
+virHostdevDeleteMissingPCIDevices(mgr, hostdevs, nhostdevs);
 }
 
 
-- 
2.26.2

[PATCH RESEND 11/20] qemu_driver.c: modernize qemuNodeDeviceReAttach()

[Daniel Henrique Barboza]

Add virObjectUnref an autoptr cleanup func for virNodeDevice,
then remove all unref and free calls from qemuNodeDeviceReAttach().

Signed-off-by: Daniel Henrique Barboza 
---
 src/datatypes.h|  2 ++
 src/qemu/qemu_driver.c | 32 
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/src/datatypes.h b/src/datatypes.h
index ade3779e43..7a88aba0df 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -707,6 +707,8 @@ struct _virNodeDevice {
 char *parentName;   /* parent device name */
 };
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevice, virObjectUnref);
+
 /**
  * _virSecret:
  *
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7581e3c8cb..f9aa93fa3e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12045,17 +12045,16 @@ static int
 qemuNodeDeviceReAttach(virNodeDevicePtr dev)
 {
 virQEMUDriverPtr driver = dev->conn->privateData;
-virPCIDevicePtr pci = NULL;
+g_autoptr(virPCIDevice) pci = NULL;
 virPCIDeviceAddress devAddr;
-int ret = -1;
-virNodeDeviceDefPtr def = NULL;
+g_autoptr(virNodeDeviceDef) def = NULL;
 g_autofree char *xml = NULL;
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
-virConnectPtr nodeconn = NULL;
-virNodeDevicePtr nodedev = NULL;
+g_autoptr(virConnect) nodeconn = NULL;
+g_autoptr(virNodeDevice) nodedev = NULL;
 
 if (!(nodeconn = virGetConnectNodeDev()))
-goto cleanup;
+return -1;
 
 /* 'dev' is associated with the QEMU virConnectPtr,
  * so for split daemons, we need to get a copy that
@@ -12063,36 +12062,29 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev)
  */
 if (!(nodedev = virNodeDeviceLookupByName(
   nodeconn, virNodeDeviceGetName(dev
-goto cleanup;
+return -1;
 
 xml = virNodeDeviceGetXMLDesc(nodedev, 0);
 if (!xml)
-goto cleanup;
+return -1;
 
 def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
 if (!def)
-goto cleanup;
+return -1;
 
 /* ACL check must happen against original 'dev',
  * not the new 'nodedev' we acquired */
 if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
-goto cleanup;
+return -1;
 
 if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
-goto cleanup;
+return -1;
 
 pci = virPCIDeviceNew();
 if (!pci)
-goto cleanup;
-
-ret = virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci);
+return -1;
 
-virPCIDeviceFree(pci);
- cleanup:
-virNodeDeviceDefFree(def);
-virObjectUnref(nodedev);
-virObjectUnref(nodeconn);
-return ret;
+return virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci);
 }
 
 static int
-- 
2.26.2

[PATCH RESEND 12/20] libxl_driver.c: modernize libxlNodeDeviceReAttach()

[Daniel Henrique Barboza]

Use g_auto* wherever we can and remove the 'cleanup' label.

Signed-off-by: Daniel Henrique Barboza 
---
 src/libxl/libxl_driver.c | 37 ++---
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3eaf106006..fbb67e9ed6 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5852,18 +5852,17 @@ libxlNodeDeviceDettach(virNodeDevicePtr dev)
 static int
 libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 {
-virPCIDevicePtr pci = NULL;
+g_autoptr(virPCIDevice) pci = NULL;
 virPCIDeviceAddress devAddr;
-int ret = -1;
-virNodeDeviceDefPtr def = NULL;
-char *xml = NULL;
+g_autoptr(virNodeDeviceDef) def = NULL;
+g_autofree char *xml = NULL;
 libxlDriverPrivatePtr driver = dev->conn->privateData;
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
-virConnectPtr nodeconn = NULL;
-virNodeDevicePtr nodedev = NULL;
+g_autoptr(virConnect) nodeconn = NULL;
+g_autoptr(virNodeDevice) nodedev = NULL;
 
 if (!(nodeconn = virGetConnectNodeDev()))
-goto cleanup;
+return -1;
 
 /* 'dev' is associated with the QEMU virConnectPtr,
  * so for split daemons, we need to get a copy that
@@ -5871,40 +5870,32 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
  */
 if (!(nodedev = virNodeDeviceLookupByName(
   nodeconn, virNodeDeviceGetName(dev
-goto cleanup;
+return -1;
 
 xml = virNodeDeviceGetXMLDesc(nodedev, 0);
 if (!xml)
-goto cleanup;
+return -1;
 
 def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
 if (!def)
-goto cleanup;
+return -1;
 
 /* ACL check must happen against original 'dev',
  * not the new 'nodedev' we acquired */
 if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
-goto cleanup;
+return -1;
 
 if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
-goto cleanup;
+return -1;
 
 pci = virPCIDeviceNew();
 if (!pci)
-goto cleanup;
+return -1;
 
 if (virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci) < 0)
-goto cleanup;
-
-ret = 0;
+return -1;
 
- cleanup:
-virPCIDeviceFree(pci);
-virNodeDeviceDefFree(def);
-virObjectUnref(nodedev);
-virObjectUnref(nodeconn);
-VIR_FREE(xml);
-return ret;
+return 0;
 }
 
 static int
-- 
2.26.2

[PATCH RESEND 15/20] qemu_cgroup.c: skip absent PCI devices in qemuTeardownHostdevCgroup()

[Daniel Henrique Barboza]

There is no need to bother with cgroup tearing down for absent
PCI devices, given that their entries in the sysfs are already
gone.

Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_cgroup.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f7146a71c9..050df21d87 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -467,6 +467,16 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
 if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
 return 0;
 
+/* Skip tearing down Cgroup for hostdevs that represents absent
+ * PCI devices, e.g. SR-IOV virtual functions that were removed from
+ * the host while the domain was still running. */
+if (virHostdevIsPCIDevice(dev)) {
+const virDomainHostdevSubsysPCI *pcisrc = >source.subsys.u.pci;
+
+if (!virPCIDeviceExists(>addr))
+return 0;
+}
+
 if (qemuDomainGetHostdevPath(dev, , NULL) < 0)
 return -1;
 
-- 
2.26.2

[PATCH RESEND 18/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListSteal()

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 8 +---
 src/util/virpci.c   | 6 +++---
 src/util/virpci.h   | 2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 3bfb04c674..4042f874d6 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -846,7 +846,7 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
 
 VIR_DEBUG("Removing PCI device %s from inactive list",
   virPCIDeviceGetName(pci));
-actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci);
+actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, 
virPCIDeviceGetAddress(pci));
 
 VIR_DEBUG("Adding PCI device %s to active list",
   virPCIDeviceGetName(pci));
@@ -918,7 +918,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
 
 VIR_DEBUG("Removing PCI device %s from active list",
   virPCIDeviceGetName(pci));
-if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci)))
+if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs,
+ virPCIDeviceGetAddress(pci
 continue;
 
 VIR_DEBUG("Adding PCI device %s to inactive list",
@@ -1022,7 +1023,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr,
 
 VIR_DEBUG("Removing PCI device %s from active list",
   virPCIDeviceGetName(pci));
-actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci);
+actual = virPCIDeviceListSteal(mgr->activePCIHostdevs,
+   virPCIDeviceGetAddress(pci));
 
 VIR_DEBUG("Adding PCI device %s to inactive list",
   virPCIDeviceGetName(pci));
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9544275c31..332e681c43 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1743,16 +1743,16 @@ virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
 
 virPCIDevicePtr
 virPCIDeviceListSteal(virPCIDeviceListPtr list,
-  virPCIDevicePtr dev)
+  virPCIDeviceAddressPtr devAddr)
 {
-return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
>address));
+return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, 
devAddr));
 }
 
 void
 virPCIDeviceListDel(virPCIDeviceListPtr list,
 virPCIDevicePtr dev)
 {
-virPCIDeviceFree(virPCIDeviceListSteal(list, dev));
+virPCIDeviceFree(virPCIDeviceListSteal(list, >address));
 }
 
 int
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 628a293972..e3458a75fa 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -161,7 +161,7 @@ virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr 
list,
 int idx);
 size_t virPCIDeviceListCount(virPCIDeviceListPtr list);
 virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list,
-  virPCIDevicePtr dev);
+  virPCIDeviceAddressPtr devAddr);
 virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
int idx);
 void virPCIDeviceListDel(virPCIDeviceListPtr list,
-- 
2.26.2

[PATCH RESEND 03/20] domain_driver.c: use PCI address with virDomainDriverNodeDeviceGetPCIInfo()

[Daniel Henrique Barboza]

Instead of receiving 4 uints in order and write domain/bus/slot/function,
receive a virPCIDeviceAddressPtr instead and write into it.

This change will allow us to simplify the API for virPCIDeviceNew()
in the next patch.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/domain_driver.c | 13 +
 src/hypervisor/domain_driver.h |  5 +
 src/libxl/libxl_driver.c   | 18 +-
 src/qemu/qemu_driver.c | 18 +-
 4 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
index 68dbf10ac6..cdb53bcf3e 100644
--- a/src/hypervisor/domain_driver.c
+++ b/src/hypervisor/domain_driver.c
@@ -341,20 +341,17 @@ 
virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
 
 int
 virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
-unsigned *domain,
-unsigned *bus,
-unsigned *slot,
-unsigned *function)
+virPCIDeviceAddressPtr devAddr)
 {
 virNodeDevCapsDefPtr cap;
 
 cap = def->caps;
 while (cap) {
 if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-*domain   = cap->data.pci_dev.domain;
-*bus  = cap->data.pci_dev.bus;
-*slot = cap->data.pci_dev.slot;
-*function = cap->data.pci_dev.function;
+devAddr->domain = cap->data.pci_dev.domain;
+devAddr->bus = cap->data.pci_dev.bus;
+devAddr->slot = cap->data.pci_dev.slot;
+devAddr->function = cap->data.pci_dev.function;
 break;
 }
 
diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
index 2bb053d559..86b92d0284 100644
--- a/src/hypervisor/domain_driver.h
+++ b/src/hypervisor/domain_driver.h
@@ -48,7 +48,4 @@ int 
virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
  int nparams);
 
 int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
-unsigned *domain,
-unsigned *bus,
-unsigned *slot,
-unsigned *function);
+virPCIDeviceAddressPtr devAddr);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0821d39c9b..360d553a22 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5780,7 +5780,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
unsigned int flags)
 {
 virPCIDevicePtr pci = NULL;
-unsigned domain = 0, bus = 0, slot = 0, function = 0;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 virNodeDeviceDefPtr def = NULL;
 char *xml = NULL;
@@ -5815,10 +5815,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
 if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (virDomainDriverNodeDeviceGetPCIInfo(def, , , , 
) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(domain, bus, slot, function);
+pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
 if (!pci)
 goto cleanup;
 
@@ -5853,7 +5853,7 @@ static int
 libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 {
 virPCIDevicePtr pci = NULL;
-unsigned domain = 0, bus = 0, slot = 0, function = 0;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 virNodeDeviceDefPtr def = NULL;
 char *xml = NULL;
@@ -5886,10 +5886,10 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (virDomainDriverNodeDeviceGetPCIInfo(def, , , , 
) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(domain, bus, slot, function);
+pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
 if (!pci)
 goto cleanup;
 
@@ -5911,7 +5911,7 @@ static int
 libxlNodeDeviceReset(virNodeDevicePtr dev)
 {
 virPCIDevicePtr pci = NULL;
-unsigned domain = 0, bus = 0, slot = 0, function = 0;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 virNodeDeviceDefPtr def = NULL;
 char *xml = NULL;
@@ -5944,10 +5944,10 @@ libxlNodeDeviceReset(virNodeDevicePtr dev)
 if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (virDomainDriverNodeDeviceGetPCIInfo(def, , , , 
) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(domain, bus, slot, function);
+pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
 if (!pci)
 

[PATCH RESEND 17/20] virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFind()

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 12 
 src/util/virpci.c   | 16 
 src/util/virpci.h   |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 402d7be42d..3bfb04c674 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -657,7 +657,8 @@ virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr,
 
 /* We need to look up the actual device because that's what
  * virPCIDeviceReattach() expects as its argument */
-if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
+if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs,
+virPCIDeviceGetAddress(pci
 continue;
 
 if (virPCIDeviceGetManaged(actual)) {
@@ -777,7 +778,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
 
 /* Unmanaged devices should already have been marked as
  * inactive: if that's the case, we can simply move on */
-if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) {
+if (virPCIDeviceListFind(mgr->inactivePCIHostdevs,
+ virPCIDeviceGetAddress(pci))) {
 VIR_DEBUG("Not detaching unmanaged PCI device %s",
   virPCIDeviceGetName(pci));
 continue;
@@ -860,7 +862,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
  * there because 'pci' only contain address information and will
  * be released at the end of the function */
 pci = virPCIDeviceListGet(pcidevs, i);
-actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
+actual = virPCIDeviceListFind(mgr->activePCIHostdevs,
+  virPCIDeviceGetAddress(pci));
 
 VIR_DEBUG("Setting driver and domain information for PCI device %s",
   virPCIDeviceGetName(pci));
@@ -992,7 +995,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr,
  * information such as by which domain and driver it is used. As a
  * side effect, by looking it up we can also tell whether it was
  * really active in the first place */
-actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
+actual = virPCIDeviceListFind(mgr->activePCIHostdevs,
+  virPCIDeviceGetAddress(pci));
 if (actual) {
 const char *actual_drvname;
 const char *actual_domname;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 1554acffb6..9544275c31 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -705,7 +705,7 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, 
virPCIDevicePtr check, void
 return 0;
 
 /* same bus, but inactive, i.e. about to be assigned to guest */
-if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, check))
+if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, >address))
 return 0;
 
 return 1;
@@ -1022,7 +1022,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
 return -1;
 }
 
-if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
+if (activeDevs && virPCIDeviceListFind(activeDevs, >address)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not resetting active device %s"), dev->name);
 return -1;
@@ -1294,7 +1294,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
 if (virPCIProbeStubDriver(dev->stubDriver) < 0)
 return -1;
 
-if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
+if (activeDevs && virPCIDeviceListFind(activeDevs, >address)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not detaching active device %s"), dev->name);
 return -1;
@@ -1306,7 +1306,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
 /* Add *a copy of* the dev into list inactiveDevs, if
  * it's not already there.
  */
-if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) {
+if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, >address)) {
 VIR_DEBUG("Adding PCI device %s to inactive list", dev->name);
 if (virPCIDeviceListAddCopy(inactiveDevs, dev) < 0)
 return -1;
@@ -1324,7 +1324,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
  virPCIDeviceListPtr activeDevs,
  virPCIDeviceListPtr inactiveDevs)
 {
-if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
+if (activeDevs && virPCIDeviceListFind(activeDevs, >address)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not reattaching active device %s"), dev->name);
 return -1;
@@ -1684,7 +1684,7 @@ int
 virPCIDeviceListAdd(virPCIDeviceListPtr list,
 virPCIDevicePtr dev)
 {
-if 

[PATCH RESEND 08/20] security_dac.c: modernize hostdev label set/restore functions

[Daniel Henrique Barboza]

Use g_auto* cleanup to avoid free() calls.

Signed-off-by: Daniel Henrique Barboza 
---
 src/security/security_dac.c | 49 +++--
 1 file changed, 14 insertions(+), 35 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index b5e56feaa8..0085982bb1 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1251,7 +1251,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -1262,41 +1262,35 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 ret = virUSBDeviceFileIterate(usb,
   virSecurityDACSetUSBLabel,
   );
-virUSBDeviceFree(usb);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
-if (!vfioGroupDev) {
-virPCIDeviceFree(pci);
+if (!vfioGroupDev)
 return -1;
-}
+
 ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
   false,
   );
-VIR_FREE(vfioGroupDev);
 } else {
 ret = virPCIDeviceFileIterate(pci,
   virSecurityDACSetPCILabel,
   );
 }
-
-virPCIDeviceFree(pci);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
 virDomainHostdevSubsysSCSIHostPtr scsihostsrc = >u.host;
-virSCSIDevicePtr scsi =
+g_autoptr(virSCSIDevice) scsi =
 virSCSIDeviceNew(NULL,
  scsihostsrc->adapter, scsihostsrc->bus,
  scsihostsrc->target, scsihostsrc->unit,
@@ -1308,13 +1302,11 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 ret = virSCSIDeviceFileIterate(scsi,
virSecurityDACSetSCSILabel,
);
-virSCSIDeviceFree(scsi);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: {
-virSCSIVHostDevicePtr host = virSCSIVHostDeviceNew(hostsrc->wwpn);
+g_autoptr(virSCSIVHostDevice) host = 
virSCSIVHostDeviceNew(hostsrc->wwpn);
 
 if (!host)
 return -1;
@@ -1322,19 +1314,16 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 ret = virSCSIVHostDeviceFileIterate(host,
 virSecurityDACSetHostLabel,
 );
-virSCSIVHostDeviceFree(host);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
-char *vfiodev = NULL;
+g_autofree char *vfiodev = NULL;
 
 if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr)))
 return -1;
 
 ret = virSecurityDACSetHostdevLabelHelper(vfiodev, true, );
-
-VIR_FREE(vfiodev);
 break;
 }
 
@@ -1420,7 +1409,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -1429,20 +1418,17 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 return -1;
 
 ret = virUSBDeviceFileIterate(usb, virSecurityDACRestoreUSBLabel, mgr);
-virUSBDeviceFree(usb);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
 if (!vfioGroupDev) {
 virPCIDeviceFree(pci);
@@ -1450,17 +1436,15 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 }
 ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
 

[PATCH RESEND 09/20] dac, selinux: skip setting/restoring label for absent PCI devices

[Daniel Henrique Barboza]

If the underlying PCI device of a hostdev does not exist in the
host (e.g. a SR-IOV VF that was removed while the domain was
running), skip security label handling for it.

This will avoid errors that happens during qemuProcessStop() time,
where a VF that was being used by the domain is not present anymore.
The restore label functions of both DAC and SELinux drivers will
trigger errors in virPCIDeviceNew().

Signed-off-by: Daniel Henrique Barboza 
---
 src/security/security_dac.c | 14 --
 src/security/security_selinux.c | 14 --
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0085982bb1..a2528aeb2d 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1266,7 +1266,12 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(>addr))
+break;
+
+pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
@@ -1422,7 +1427,12 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr 
mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(>addr))
+break;
+
+pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 99adf08a15..c018c0708a 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2101,7 +2101,12 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(>addr))
+break;
+
+pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
@@ -2329,7 +2334,12 @@ 
virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = NULL;
+
+if (!virPCIDeviceExists(>addr))
+break;
+
+pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
-- 
2.26.2

[PATCH RESEND 13/20] virsh-domain.c: modernize cmdDetachDevice()

[Daniel Henrique Barboza]

Use g_auto* pointers to avoid the need of a cleanup label. The
type of the pointer 'virDomainPtr dom' was changed to its alias
'virshDomainPtr' to allow the use of g_autoptr().

Signed-off-by: Daniel Henrique Barboza 
---
 tools/virsh-domain.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 2bb136333f..b32529f073 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11953,11 +11953,10 @@ static const vshCmdOptDef opts_detach_device[] = {
 static bool
 cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
 {
-virDomainPtr dom = NULL;
+g_autoptr(virshDomain) dom = NULL;
 const char *from = NULL;
-char *buffer = NULL;
+g_autofree char *buffer = NULL;
 int ret;
-bool funcRet = false;
 bool current = vshCommandOptBool(cmd, "current");
 bool config = vshCommandOptBool(cmd, "config");
 bool live = vshCommandOptBool(cmd, "live");
@@ -11982,11 +11981,11 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_AFFECT_LIVE;
 
 if (vshCommandOptStringReq(ctl, cmd, "file", ) < 0)
-goto cleanup;
+return false;
 
 if (virFileReadAll(from, VSH_MAX_XML_FILE, ) < 0) {
 vshReportError(ctl);
-goto cleanup;
+return false;
 }
 
 if (flags != 0 || current)
@@ -11996,16 +11995,11 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
 
 if (ret < 0) {
 vshError(ctl, _("Failed to detach device from %s"), from);
-goto cleanup;
+return false;
 }
 
 vshPrintExtra(ctl, "%s", _("Device detached successfully\n"));
-funcRet = true;
-
- cleanup:
-VIR_FREE(buffer);
-virshDomainFree(dom);
-return funcRet;
+return true;
 }
 
 
-- 
2.26.2

[PATCH RESEND 07/20] security_selinux.c: modernize set/restore hostdev subsys label functions

[Daniel Henrique Barboza]

Use g_auto* cleanup to avoid free() calls.

Signed-off-by: Daniel Henrique Barboza 
---
 src/security/security_selinux.c | 54 ++---
 1 file changed, 16 insertions(+), 38 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 76edaed027..99adf08a15 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2085,7 +2085,7 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -2097,39 +2097,34 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 return -1;
 
 ret = virUSBDeviceFileIterate(usb, virSecuritySELinuxSetUSBLabel, 
);
-virUSBDeviceFree(usb);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
-if (!vfioGroupDev) {
-virPCIDeviceFree(pci);
+if (!vfioGroupDev)
 return -1;
-}
+
 ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
   false,
   );
-VIR_FREE(vfioGroupDev);
 } else {
 ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetPCILabel, 
);
 }
-virPCIDeviceFree(pci);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
 virDomainHostdevSubsysSCSIHostPtr scsihostsrc = >u.host;
 
-virSCSIDevicePtr scsi =
+g_autoptr(virSCSIDevice) scsi =
 virSCSIDeviceNew(NULL,
  scsihostsrc->adapter, scsihostsrc->bus,
  scsihostsrc->target, scsihostsrc->unit,
@@ -2141,13 +2136,11 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 ret = virSCSIDeviceFileIterate(scsi,
virSecuritySELinuxSetSCSILabel,
);
-virSCSIDeviceFree(scsi);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: {
-virSCSIVHostDevicePtr host = virSCSIVHostDeviceNew(hostsrc->wwpn);
+g_autoptr(virSCSIVHostDevice) host = 
virSCSIVHostDeviceNew(hostsrc->wwpn);
 
 if (!host)
 return -1;
@@ -2155,19 +2148,16 @@ 
virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
 ret = virSCSIVHostDeviceFileIterate(host,
 virSecuritySELinuxSetHostLabel,
 );
-virSCSIVHostDeviceFree(host);
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
-char *vfiodev = NULL;
+g_autofree char *vfiodev = NULL;
 
 if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr)))
 return ret;
 
 ret = virSecuritySELinuxSetHostdevLabelHelper(vfiodev, true, );
-
-VIR_FREE(vfiodev);
 break;
 }
 
@@ -2323,7 +2313,7 @@ 
virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
 
 switch ((virDomainHostdevSubsysType)dev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb;
+g_autoptr(virUSBDevice) usb = NULL;
 
 if (dev->missing)
 return 0;
@@ -2335,37 +2325,31 @@ 
virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
 return -1;
 
 ret = virUSBDeviceFileIterate(usb, virSecuritySELinuxRestoreUSBLabel, 
mgr);
-virUSBDeviceFree(usb);
-
 break;
 }
 
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-virPCIDevicePtr pci =
-virPCIDeviceNew(>addr);
+g_autoptr(virPCIDevice) pci = virPCIDeviceNew(>addr);
 
 if (!pci)
 return -1;
 
 if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
-char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
 
-if (!vfioGroupDev) {
-virPCIDeviceFree(pci);
+if (!vfioGroupDev)
 return -1;
-}
+
 ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false);
-VIR_FREE(vfioGroupDev);
 } else {
 ret = 

[PATCH RESEND 06/20] virhostdev.c: virHostdevGetPCIHostDevice() now reports missing device

[Daniel Henrique Barboza]

Gitlab issue #72 [1] reports that removing SR-IOVs VFs before
removing the devices from the running domains can have strange
consequences. QEMU might be able to hotunplug the device inside the
guest, but Libvirt will not be aware of that, and then the guest is
now inconsistent with the domain definition.

There's also the possibility of the VFs removal not succeeding
while the domain is running but then, as soon as the domain
is shutdown, all the VFs are removed. Libvirt can't handle
the removal of the PCI devices while trying to reattach the
hostdevs, and the Libvirt daemon can be left in an inconsistent
state (see [2]).

This patch starts to address the issue related in Gitlab #72, most
notably the issue described in [2]. When shutting down a domain
with SR-IOV hostdevs that got missing, virHostdevReAttachPCIDevices()
is failing the whole process and failing to reattach all the
PCI devices, including the ones that aren't related to the VFs that
went missing. Let's make it more resilient with host changes by
changing virHostdevGetPCIHostDevice() to return an exclusive error
code '-2' for this case. virHostdevGetPCIHostDeviceList() can then
tell when virHostdevGetPCIHostDevice() failed to find the PCI
device of a hostdev and continue to make the list of PCI devices.

virHostdevReAttachPCIDevices() will now be able to proceed reattaching
all other valid PCI devices, at least. The 'ghost hostdevs' will be
handled later on.

[1] https://gitlab.com/libvirt/libvirt/-/issues/72
[2] https://gitlab.com/libvirt/libvirt/-/issues/72#note_459032148

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index bd35397f2c..dbba36193b 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -220,7 +220,8 @@ virHostdevManagerGetDefault(void)
  * is returned.
  *
  * Returns: 0 on success (@pci might be NULL though),
- * -1 otherwise (with error reported).
+ * -1 otherwise (with error reported),
+ * -2 PCI device not found. @pci will be NULL
  */
 static int
 virHostdevGetPCIHostDevice(const virDomainHostdevDef *hostdev,
@@ -235,6 +236,9 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef 
*hostdev,
 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
 return 0;
 
+if (!virPCIDeviceExists(>addr))
+return -2;
+
 actual = virPCIDeviceNew(>addr);
 
 if (!actual)
@@ -270,7 +274,7 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr 
*hostdevs, int nhostdevs)
 virDomainHostdevDefPtr hostdev = hostdevs[i];
 g_autoptr(virPCIDevice) pci = NULL;
 
-if (virHostdevGetPCIHostDevice(hostdev, ) < 0)
+if (virHostdevGetPCIHostDevice(hostdev, ) == -1)
 return NULL;
 
 if (!pci)
-- 
2.26.2

[PATCH RESEND 04/20] virpci.c: simplify virPCIDeviceNew() signature

[Daniel Henrique Barboza]

The current virPCIDeviceNew() signature, receiving 4 uints in sequence
(domain, bus, slot, function), is not neat.

We already have a way to represent a PCI address in virPCIDeviceAddress
that is used in the code. Aside from the test files, most of
virPCIDeviceNew() callers have access to a virPCIDeviceAddress reference,
but then we need to retrieve the 4 required uints (addr.domain, addr.bus,
addr.slot, addr.function) to satisfy virPCIDeviceNew(). The result is
that we have extra verbosity/boilerplate to retrieve an information that
is already available in virPCIDeviceAddress.

A better way is presented by virNVMEDeviceNew(), where the caller just
supplies a virPCIDeviceAddress pointer and the function handles the
details internally.

This patch changes virPCIDeviceNew() to receive a virPCIDeviceAddress
pointer instead of 4 uints.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/virhostdev.c|  3 +--
 src/libxl/libxl_driver.c   |  6 ++---
 src/node_device/node_device_udev.c | 11 +---
 src/qemu/qemu_domain_address.c |  5 +---
 src/qemu/qemu_driver.c |  6 ++---
 src/security/security_apparmor.c   |  3 +--
 src/security/security_dac.c|  6 ++---
 src/security/security_selinux.c|  6 ++---
 src/security/virt-aa-helper.c  |  6 +
 src/util/virnetdev.c   |  3 +--
 src/util/virnvme.c |  5 +---
 src/util/virpci.c  | 40 +-
 src/util/virpci.h  |  5 +---
 tests/virhostdevtest.c |  3 ++-
 tests/virpcitest.c | 35 +++---
 15 files changed, 64 insertions(+), 79 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index be32a26164..bd35397f2c 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -235,8 +235,7 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef 
*hostdev,
 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
 return 0;
 
-actual = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
- pcisrc->addr.slot, pcisrc->addr.function);
+actual = virPCIDeviceNew(>addr);
 
 if (!actual)
 return -1;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 360d553a22..3eaf106006 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5818,7 +5818,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
 if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
+pci = virPCIDeviceNew();
 if (!pci)
 goto cleanup;
 
@@ -5889,7 +5889,7 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
 if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
+pci = virPCIDeviceNew();
 if (!pci)
 goto cleanup;
 
@@ -5947,7 +5947,7 @@ libxlNodeDeviceReset(virNodeDevicePtr dev)
 if (virDomainDriverNodeDeviceGetPCIInfo(def, ) < 0)
 goto cleanup;
 
-pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, 
devAddr.function);
+pci = virPCIDeviceNew();
 if (!pci)
 goto cleanup;
 
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 55a2731681..fceb135aa5 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -367,6 +367,7 @@ udevProcessPCI(struct udev_device *device,
 virNodeDevCapPCIDevPtr pci_dev = >caps->data.pci_dev;
 virPCIEDeviceInfoPtr pci_express = NULL;
 virPCIDevicePtr pciDev = NULL;
+virPCIDeviceAddress devAddr;
 int ret = -1;
 char *p;
 bool privileged;
@@ -416,10 +417,12 @@ udevProcessPCI(struct udev_device *device,
 if (virNodeDeviceGetPCIDynamicCaps(def->sysfs_path, pci_dev) < 0)
 goto cleanup;
 
-if (!(pciDev = virPCIDeviceNew(pci_dev->domain,
-   pci_dev->bus,
-   pci_dev->slot,
-   pci_dev->function)))
+devAddr.domain = pci_dev->domain;
+devAddr.bus = pci_dev->bus;
+devAddr.slot = pci_dev->slot;
+devAddr.function = pci_dev->function;
+
+if (!(pciDev = virPCIDeviceNew()))
 goto cleanup;
 
 /* We need to be root to read PCI device configs */
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f0ba318cc8..ae4ad6677c 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -857,10 +857,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 return 0;
 }
 
-if (!(pciDev = virPCIDeviceNew(hostAddr->domain,
-   hostAddr->bus,
-   hostAddr->slot,
-  

[PATCH RESEND 05/20] virpci: introduce virPCIDeviceExists()

[Daniel Henrique Barboza]

We're going to add logic to handle the case where a previously
existing PCI device does not longer exist in the host.

The logic was copied from virPCIDeviceNew(), which verifies if a
PCI device exists in the host, returning NULL and throwing an
error if it doesn't. The NULL is used for other errors as well
(product/vendor id read errors, dev id overflow), meaning that we
can't re-use virPCIDeviceNew() for the purpose of detecting
if the device exists.

Signed-off-by: Daniel Henrique Barboza 
---
 src/libvirt_private.syms |  1 +
 src/util/virpci.c| 10 ++
 src/util/virpci.h|  1 +
 3 files changed, 12 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 55284577b0..36635e6de7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2819,6 +2819,7 @@ virPCIDeviceAddressIsValid;
 virPCIDeviceAddressParse;
 virPCIDeviceCopy;
 virPCIDeviceDetach;
+virPCIDeviceExists;
 virPCIDeviceFileIterate;
 virPCIDeviceFree;
 virPCIDeviceGetAddress;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 5a91553b5f..7143380348 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1448,6 +1448,16 @@ virPCIDeviceAddressAsString(const virPCIDeviceAddress 
*addr)
 return str;
 }
 
+bool
+virPCIDeviceExists(const virPCIDeviceAddress *addr)
+{
+g_autofree char *devName = virPCIDeviceAddressAsString(addr);
+g_autofree char *devPath = g_strdup_printf(PCI_SYSFS "devices/%s/config",
+   devName);
+
+return virFileExists(devPath);
+}
+
 virPCIDevicePtr
 virPCIDeviceNew(const virPCIDeviceAddress *address)
 {
diff --git a/src/util/virpci.h b/src/util/virpci.h
index d4451848c1..a9c597a428 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -201,6 +201,7 @@ int 
virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
   size_t *nIommuGroupDevices);
 int virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr);
 char *virPCIDeviceAddressGetIOMMUGroupDev(const virPCIDeviceAddress *devAddr);
+bool virPCIDeviceExists(const virPCIDeviceAddress *addr);
 char *virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev);
 
 int virPCIDeviceIsAssignable(virPCIDevicePtr dev,
-- 
2.26.2

[PATCH RESEND 10/20] libvirt-nodedev.c: remove return value from virNodeDeviceFree()

[Daniel Henrique Barboza]

The function returns -1 on error, but no caller is actually checking
the return value. Making it 'void' makes more sense with its
current use.

Signed-off-by: Daniel Henrique Barboza 
---
 include/libvirt/libvirt-nodedev.h |  2 +-
 src/libvirt-nodedev.c | 15 +++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/libvirt/libvirt-nodedev.h 
b/include/libvirt/libvirt-nodedev.h
index eab8abf6ab..5634980a75 100644
--- a/include/libvirt/libvirt-nodedev.h
+++ b/include/libvirt/libvirt-nodedev.h
@@ -114,7 +114,7 @@ char *  virNodeDeviceGetXMLDesc 
(virNodeDevicePtr dev,
  unsigned int flags);
 
 int virNodeDeviceRef(virNodeDevicePtr dev);
-int virNodeDeviceFree   (virNodeDevicePtr dev);
+voidvirNodeDeviceFree   (virNodeDevicePtr dev);
 
 int virNodeDeviceDettach(virNodeDevicePtr dev);
 int virNodeDeviceDetachFlags(virNodeDevicePtr dev,
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index eb8c735a8c..fcca40f47b 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -445,19 +445,26 @@ virNodeDeviceListCaps(virNodeDevicePtr dev,
  * Drops a reference to the node device, freeing it if
  * this was the last reference.
  *
- * Returns the 0 for success, -1 for error.
+ * Throws a VIR_ERR_INVALID_NODE_DEVICE error if @dev is
+ * not a valid node device. Does nothing if @dev is
+ * NULL.
  */
-int
+void
 virNodeDeviceFree(virNodeDevicePtr dev)
 {
+if (!dev)
+return;
+
 VIR_DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
 
 virResetLastError();
 
-virCheckNodeDeviceReturn(dev, -1);
+virCheckNodeDeviceGoto(dev, invalid_device);
 
 virObjectUnref(dev);
-return 0;
+
+ invalid_device:
+return;
 }
 
 
-- 
2.26.2

[PATCH RESEND 01/20] virpci, domain_audit: use virPCIDeviceAddressAsString()

[Daniel Henrique Barboza]

There is no need to open code the PCI address string format
when we have a function that does exactly that.

Signed-off-by: Daniel Henrique Barboza 
---
 src/conf/domain_audit.c | 6 +-
 src/util/virpci.c   | 6 ++
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 8bc6633af4..5fa65a8078 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -361,11 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, 
virDomainHostdevDefPtr hostdev,
 case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
 switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
-address = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT,
-  pcisrc->addr.domain,
-  pcisrc->addr.bus,
-  pcisrc->addr.slot,
-  pcisrc->addr.function);
+address = virPCIDeviceAddressAsString(>addr);
 break;
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
 address = g_strdup_printf("%.3d.%.3d", usbsrc->bus, 
usbsrc->device);
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 50fd5ef7ea..5f50f92abb 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1471,8 +1471,7 @@ virPCIDeviceNew(unsigned int domain,
 dev->address.slot = slot;
 dev->address.function = function;
 
-dev->name = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, domain, bus, slot,
-function);
+dev->name = virPCIDeviceAddressAsString(>address);
 
 dev->path = g_strdup_printf(PCI_SYSFS "devices/%s/config", dev->name);
 
@@ -1998,8 +1997,7 @@ 
virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr)
 g_autofree char *groupNumStr = NULL;
 unsigned int groupNum;
 
-devName = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, addr->domain, 
addr->bus,
-  addr->slot, addr->function);
+devName = virPCIDeviceAddressAsString(addr);
 
 devPath = virPCIFile(devName, "iommu_group");
 
-- 
2.26.2

[PATCH RESEND 02/20] qemu, lxc: move NodeDeviceGetPCIInfo() function to domain_driver.c

[Daniel Henrique Barboza]

libxlNodeDeviceGetPCIInfo() and qemuNodeDeviceGetPCIInfo() are equal.
Let's move the logic to a new virDomainDriverNodeDeviceGetPCIInfo()
info to be used by libxl_driver.c and qemu_driver.c.

Signed-off-by: Daniel Henrique Barboza 
---
 src/hypervisor/domain_driver.c | 33 +
 src/hypervisor/domain_driver.h |  7 +++
 src/libvirt_private.syms   |  1 +
 src/libxl/libxl_driver.c   | 38 --
 src/qemu/qemu_driver.c | 37 +++--
 5 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
index 8dc5870a61..68dbf10ac6 100644
--- a/src/hypervisor/domain_driver.c
+++ b/src/hypervisor/domain_driver.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "domain_driver.h"
+#include "node_device_conf.h"
 #include "viralloc.h"
 #include "virstring.h"
 #include "vircrypto.h"
@@ -336,3 +337,35 @@ 
virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
 
 return ret;
 }
+
+
+int
+virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
+unsigned *domain,
+unsigned *bus,
+unsigned *slot,
+unsigned *function)
+{
+virNodeDevCapsDefPtr cap;
+
+cap = def->caps;
+while (cap) {
+if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+*domain   = cap->data.pci_dev.domain;
+*bus  = cap->data.pci_dev.bus;
+*slot = cap->data.pci_dev.slot;
+*function = cap->data.pci_dev.function;
+break;
+}
+
+cap = cap->next;
+}
+
+if (!cap) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("device %s is not a PCI device"), def->name);
+return -1;
+}
+
+return 0;
+}
diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
index b66ae2d421..2bb053d559 100644
--- a/src/hypervisor/domain_driver.h
+++ b/src/hypervisor/domain_driver.h
@@ -21,6 +21,7 @@
 #pragma once
 
 #include "domain_conf.h"
+#include "node_device_conf.h"
 
 char *
 virDomainDriverGenerateRootHash(const char *drivername,
@@ -45,3 +46,9 @@ int virDomainDriverParseBlkioDeviceStr(char *blkioDeviceStr, 
const char *type,
 int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
  virTypedParameterPtr params,
  int nparams);
+
+int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
+unsigned *domain,
+unsigned *bus,
+unsigned *slot,
+unsigned *function);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c325040b60..55284577b0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1445,6 +1445,7 @@ virDomainCgroupSetupMemtune;
 virDomainDriverGenerateMachineName;
 virDomainDriverGenerateRootHash;
 virDomainDriverMergeBlkioDevice;
+virDomainDriverNodeDeviceGetPCIInfo;
 virDomainDriverParseBlkioDeviceStr;
 virDomainDriverSetupPersistentDefBlkioParams;
 
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 5bd3614e21..0821d39c9b 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -56,6 +56,7 @@
 #include "cpu/cpu.h"
 #include "virutil.h"
 #include "domain_validate.h"
+#include "domain_driver.h"
 
 #define VIR_FROM_THIS VIR_FROM_LIBXL
 
@@ -5773,37 +5774,6 @@ libxlConnectSupportsFeature(virConnectPtr conn, int 
feature)
 }
 }
 
-static int
-libxlNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
-  unsigned *domain,
-  unsigned *bus,
-  unsigned *slot,
-  unsigned *function)
-{
-virNodeDevCapsDefPtr cap;
-
-cap = def->caps;
-while (cap) {
-if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-*domain   = cap->data.pci_dev.domain;
-*bus  = cap->data.pci_dev.bus;
-*slot = cap->data.pci_dev.slot;
-*function = cap->data.pci_dev.function;
-break;
-}
-
-cap = cap->next;
-}
-
-if (!cap) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("device %s is not a PCI device"), def->name);
-return -1;
-}
-
-return 0;
-}
-
 static int
 libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
const char *driverName,
@@ -5845,7 +5815,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
 if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
 goto cleanup;
 
-if (libxlNodeDeviceGetPCIInfo(def, , , , ) < 0)
+if (virDomainDriverNodeDeviceGetPCIInfo(def, , , , 
) < 0)
 goto 

[PATCH RESEND 00/20] handle missing SR-IOV VF hostdevs during running domains

[Daniel Henrique Barboza]

This is the rebased version of

https://www.redhat.com/archives/libvir-list/2021-January/msg00028.html

with master at 57b1ddcaaa5b5. No changes made aside from trivial conflicts
fixes.

I also removed the military jargon from the previous subject to make it
clear what this series is doing.

Daniel Henrique Barboza (20):
  virpci, domain_audit: use virPCIDeviceAddressAsString()
  qemu, lxc: move NodeDeviceGetPCIInfo() function to domain_driver.c
  domain_driver.c: use PCI address with
virDomainDriverNodeDeviceGetPCIInfo()
  virpci.c: simplify virPCIDeviceNew() signature
  virpci: introduce virPCIDeviceExists()
  virhostdev.c: virHostdevGetPCIHostDevice() now reports missing device
  security_selinux.c: modernize set/restore hostdev subsys label
functions
  security_dac.c: modernize hostdev label set/restore functions
  dac, selinux: skip setting/restoring label for absent PCI devices
  libvirt-nodedev.c: remove return value from virNodeDeviceFree()
  qemu_driver.c: modernize qemuNodeDeviceReAttach()
  libxl_driver.c: modernize libxlNodeDeviceReAttach()
  virsh-domain.c: modernize cmdDetachDevice()
  virhostdev.c: add virHostdevIsPCIDevice() helper
  qemu_cgroup.c: skip absent PCI devices in qemuTeardownHostdevCgroup()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFindIndex()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListFind()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListSteal()
  virpci.c: use virPCIDeviceAddressPtr in virPCIDeviceListDel()
  virhostdev.c: remove missing PCI devs from hostdev manager

 include/libvirt/libvirt-nodedev.h  |  2 +-
 src/conf/domain_audit.c|  6 +-
 src/datatypes.h|  2 +
 src/hypervisor/domain_driver.c | 30 ++
 src/hypervisor/domain_driver.h |  4 ++
 src/hypervisor/virhostdev.c| 88 ++--
 src/hypervisor/virhostdev.h|  2 +
 src/libvirt-nodedev.c  | 15 +++--
 src/libvirt_private.syms   |  3 +
 src/libxl/libxl_driver.c   | 87 
 src/node_device/node_device_udev.c | 11 ++--
 src/qemu/qemu_cgroup.c | 10 
 src/qemu/qemu_domain_address.c |  5 +-
 src/qemu/qemu_driver.c | 81 +++---
 src/security/security_apparmor.c   |  3 +-
 src/security/security_dac.c| 61 
 src/security/security_selinux.c| 66 +
 src/security/virt-aa-helper.c  |  6 +-
 src/util/virnetdev.c   |  3 +-
 src/util/virnvme.c |  5 +-
 src/util/virpci.c  | 93 ++
 src/util/virpci.h  | 14 ++---
 tests/virhostdevtest.c |  3 +-
 tests/virpcitest.c | 35 ---
 tools/virsh-domain.c   | 18 ++
 25 files changed, 325 insertions(+), 328 deletions(-)

-- 
2.26.2

[PATCH] meson: fix vstorage driver build

[Nikolay Shirokovskiy]

It breaks on using - in VSTORAGE-MOUNT definition with:

  In file included from ../config.h:19:0,
  from ../src/util/viraudit.c:22:
  ./meson-config.h:180:17: error: ISO C99 requires whitespace after the macro 
name [-Werror]
  #define VSTORAGE-MOUNT "/usr/bin/vstorage-mount"
  ^
  ./meson-config.h:180:0: error: "VSTORAGE" redefined [-Werror]
  #define VSTORAGE-MOUNT "/usr/bin/vstorage-mount"
  ^
  ./meson-config.h:178:0: note: this is the location of the previous definition
  #define VSTORAGE "/usr/bin/vstorage"
  ^
  #define VSTORAGE-MOUNT "/usr/bin/vstorage-mount"
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index b5277b4..aff2565 100644
--- a/meson.build
+++ b/meson.build
@@ -1974,7 +1974,7 @@ if conf.has('WITH_LIBVIRTD')
   conf.set('WITH_STORAGE_VSTORAGE', 1)
   foreach name : ['vstorage', 'vstorage-mount', 'umount']
 path = get_variable('@0@_prog'.format(name.underscorify())).path()
-conf.set_quoted(name.to_upper(), path)
+conf.set_quoted(name.underscorify().to_upper(), path)
   endforeach
 endif
   endif
-- 
1.8.3.1

[libvirt PATCH] qemu_validate: Allow kvm hint-dedicated on non-passthrough VMs

[Tim Wiederhake]

A VM defined similar to:
  ...
  
  
  ...
is currently invalid, as hint-dedicated is only allowed if cpu mode
is host-passthrough. This restriction is unnecessary, see
https://bugzilla.redhat.com/show_bug.cgi?id=1857671.

Signed-off-by: Tim Wiederhake 
---
 src/qemu/qemu_validate.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index a060bd98ba..c58d221cf8 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -253,16 +253,6 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
 }
 break;
 
-case VIR_DOMAIN_FEATURE_KVM:
-if (def->kvm_features[VIR_DOMAIN_KVM_DEDICATED] == 
VIR_TRISTATE_SWITCH_ON &&
-(!def->cpu || def->cpu->mode != 
VIR_CPU_MODE_HOST_PASSTHROUGH)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("kvm-hint-dedicated=on is only applicable "
- "for cpu host-passthrough"));
-return -1;
-}
-break;
-
 case VIR_DOMAIN_FEATURE_VMPORT:
 if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
 !virQEMUCapsSupportsVmport(qemuCaps, def)) {
@@ -335,6 +325,7 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_KVM:
 case VIR_DOMAIN_FEATURE_XEN:
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
-- 
2.26.2

Re: [PATCH 2/2] conf: Move generation of NVDIMM UUID into post parse callback

[Peter Krempa]

On Mon, Jan 18, 2021 at 15:15:54 +0100, Michal Privoznik wrote:
> It's better to fill in missing values in post parse callbacks
> than during parsing.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c | 35 ++-
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a2ddfcf947..4f0798de45 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

[...]

> @@ -5345,6 +5346,19 @@ virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem)
>  break;
>  
>  case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +/* If no NVDIMM UUID was provided in XML, generate one. */
> +if (ARCH_IS_PPC64(def->os.arch) &&
> +!mem->uuid) {
> +
> +mem->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN);
> +if (virUUIDGenerate(mem->uuid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s", _("Failed to generate UUID"));
> +return -1;
> +}
> +}


You can also reject if an UUID is present but the architecture isn't
PPC64 here, rather than ...

> +break;
> +
>  case VIR_DOMAIN_MEMORY_MODEL_DIMM:
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:

[...]

> @@ -15526,24 +15540,19 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr 
> xmlopt,
>  }
>  VIR_FREE(tmp);
>  
> +/* Extract NVDIMM UUID. */
>  if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
> -ARCH_IS_PPC64(dom->os.arch)) {
> -/* Extract nvdimm uuid or generate a new one */
> -tmp = virXPathString("string(./uuid[1])", ctxt);
> -
> +ARCH_IS_PPC64(dom->os.arch) &&

... keeping it in the parser, where it doesn't make much sense.

> +(tmp = virXPathString("string(./uuid[1])", ctxt))) {
>  def->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN);
> -if (!tmp) {
> -if (virUUIDGenerate(def->uuid) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   "%s", _("Failed to generate UUID"));
> -goto error;
> -}
> -} else if (virUUIDParse(tmp, def->uuid) < 0) {
> +

Reviewed-by: Peter Krempa 

Re: [PATCH 1/2] conf: Turn @uuid member of _virDomainMemoryDef struct into a pointer

[Peter Krempa]

On Mon, Jan 18, 2021 at 15:15:53 +0100, Michal Privoznik wrote:
> The _virDomainMemoryDef structure has @uuid member which is
> needed for PPC64 guests. No other architectures use it. Since the
> member is VIR_UUID_BUFLEN bytes long, the structure is
> unnecessary big. If the member is just a pointer then we can also

Umm, this saves just 8 bytes. (pointer is 8, VIR_UUID_BUFLEN 16)

> replace some calls of virUUIDIsValid() with plain test against
> NULL.

This commit isn't replacing them though (which is okay), but it also
moves logic out of the XML formatter where we no longer need to check
for the correct architecture, which isn't mentioned.

> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c  | 14 --
>  src/conf/domain_conf.h  |  2 +-
>  src/qemu/qemu_command.c |  2 +-
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ff3b7cbfc8..a2ddfcf947 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

[...]

> @@ -22808,7 +22810,9 @@ 
> virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
>  return false;
>  }
>  
> -if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) {
> +if ((src->uuid || dst->uuid) &&
> +!(src->uuid && dst->uuid &&
> +memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) == 0)) {

wrong alignment of the memcmp line since it's inside the second
sub-term.

>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Target NVDIMM UUID doesn't match source 
> NVDIMM"));
>  return false;

[...]

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f59d972c85..6645282bf6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2331,7 +2331,7 @@ struct _virDomainMemoryDef {
>  bool readonly; /* valid only for NVDIMM */
>  
>  /* required for QEMU NVDIMM ppc64 support */
> -unsigned char uuid[VIR_UUID_BUFLEN];
> +unsigned char *uuid; 

Please add a comment that this is expected to be VIR_UUID_BUFLEN long
buffer if allocated.

>  
>  virDomainDeviceInfo info;
>  };

Reviewed-by: Peter Krempa 

[PATCH 0/2] Move generation of NVDIMM UUID into post parse callback

[Michal Privoznik]

I've noticed this while working on virtio-pmem-pci patches.

Michal Prívozník (2):
  conf: Turn @uuid member of _virDomainMemoryDef struct into a pointer
  conf: Move generation of NVDIMM UUID into post parse callback

 src/conf/domain_conf.c  | 47 +
 src/conf/domain_conf.h  |  2 +-
 src/qemu/qemu_command.c |  2 +-
 3 files changed, 31 insertions(+), 20 deletions(-)

-- 
2.26.2

[PATCH 2/2] conf: Move generation of NVDIMM UUID into post parse callback

[Michal Privoznik]

It's better to fill in missing values in post parse callbacks
than during parsing.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 35 ++-
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a2ddfcf947..4f0798de45 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5334,7 +5334,8 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
 
 
 static int
-virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem)
+virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
+const virDomainDef *def)
 {
 switch (mem->model) {
 case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
@@ -5345,6 +5346,19 @@ virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem)
 break;
 
 case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
+/* If no NVDIMM UUID was provided in XML, generate one. */
+if (ARCH_IS_PPC64(def->os.arch) &&
+!mem->uuid) {
+
+mem->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN);
+if (virUUIDGenerate(mem->uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("Failed to generate UUID"));
+return -1;
+}
+}
+break;
+
 case VIR_DOMAIN_MEMORY_MODEL_DIMM:
 case VIR_DOMAIN_MEMORY_MODEL_NONE:
 case VIR_DOMAIN_MEMORY_MODEL_LAST:
@@ -5401,7 +5415,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr 
dev,
 break;
 
 case VIR_DOMAIN_DEVICE_MEMORY:
-ret = virDomainMemoryDefPostParse(dev->data.memory);
+ret = virDomainMemoryDefPostParse(dev->data.memory, def);
 break;
 
 case VIR_DOMAIN_DEVICE_LEASE:
@@ -15526,24 +15540,19 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr 
xmlopt,
 }
 VIR_FREE(tmp);
 
+/* Extract NVDIMM UUID. */
 if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-ARCH_IS_PPC64(dom->os.arch)) {
-/* Extract nvdimm uuid or generate a new one */
-tmp = virXPathString("string(./uuid[1])", ctxt);
-
+ARCH_IS_PPC64(dom->os.arch) &&
+(tmp = virXPathString("string(./uuid[1])", ctxt))) {
 def->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN);
-if (!tmp) {
-if (virUUIDGenerate(def->uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("Failed to generate UUID"));
-goto error;
-}
-} else if (virUUIDParse(tmp, def->uuid) < 0) {
+
+if (virUUIDParse(tmp, def->uuid) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("malformed uuid element"));
 goto error;
 }
 }
+VIR_FREE(tmp);
 
 /* source */
 if ((node = virXPathNode("./source", ctxt)) &&
-- 
2.26.2

[PATCH 1/2] conf: Turn @uuid member of _virDomainMemoryDef struct into a pointer

[Michal Privoznik]

The _virDomainMemoryDef structure has @uuid member which is
needed for PPC64 guests. No other architectures use it. Since the
member is VIR_UUID_BUFLEN bytes long, the structure is
unnecessary big. If the member is just a pointer then we can also
replace some calls of virUUIDIsValid() with plain test against
NULL.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c  | 14 --
 src/conf/domain_conf.h  |  2 +-
 src/qemu/qemu_command.c |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ff3b7cbfc8..a2ddfcf947 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3116,6 +3116,7 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def)
 
 VIR_FREE(def->nvdimmPath);
 virBitmapFree(def->sourceNodes);
+VIR_FREE(def->uuid);
 virDomainDeviceInfoClear(>info);
 VIR_FREE(def);
 }
@@ -15530,6 +15531,7 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
 /* Extract nvdimm uuid or generate a new one */
 tmp = virXPathString("string(./uuid[1])", ctxt);
 
+def->uuid = g_new0(unsigned char, VIR_UUID_BUFLEN);
 if (!tmp) {
 if (virUUIDGenerate(def->uuid) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -22808,7 +22810,9 @@ 
virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
 return false;
 }
 
-if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) {
+if ((src->uuid || dst->uuid) &&
+!(src->uuid && dst->uuid &&
+memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) == 0)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Target NVDIMM UUID doesn't match source 
NVDIMM"));
 return false;
@@ -26560,7 +26564,6 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
 static int
 virDomainMemoryDefFormat(virBufferPtr buf,
  virDomainMemoryDefPtr def,
- const virDomainDef *dom,
  unsigned int flags)
 {
 const char *model = virDomainMemoryModelTypeToString(def->model);
@@ -26575,8 +26578,7 @@ virDomainMemoryDefFormat(virBufferPtr buf,
 virBufferAddLit(buf, ">\n");
 virBufferAdjustIndent(buf, 2);
 
-if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-ARCH_IS_PPC64(dom->os.arch)) {
+if (def->uuid) {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 
 virUUIDFormat(def->uuid, uuidstr);
@@ -28974,7 +28976,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr 
def,
 virDomainShmemDefFormat(buf, def->shmems[n], flags);
 
 for (n = 0; n < def->nmems; n++) {
-if (virDomainMemoryDefFormat(buf, def->mems[n], def, flags) < 0)
+if (virDomainMemoryDefFormat(buf, def->mems[n], flags) < 0)
 return -1;
 }
 
@@ -30091,7 +30093,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
 rc = 0;
 break;
 case VIR_DOMAIN_DEVICE_MEMORY:
-rc = virDomainMemoryDefFormat(, src->data.memory, def, flags);
+rc = virDomainMemoryDefFormat(, src->data.memory, flags);
 break;
 case VIR_DOMAIN_DEVICE_SHMEM:
 virDomainShmemDefFormat(, src->data.shmem, flags);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f59d972c85..6645282bf6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2331,7 +2331,7 @@ struct _virDomainMemoryDef {
 bool readonly; /* valid only for NVDIMM */
 
 /* required for QEMU NVDIMM ppc64 support */
-unsigned char uuid[VIR_UUID_BUFLEN];
+unsigned char *uuid;
 
 virDomainDeviceInfo info;
 };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 99761c217d..2506248866 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3319,7 +3319,7 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
 if (mem->labelsize)
 virBufferAsprintf(, "label-size=%llu,", mem->labelsize * 1024);
 
-if (virUUIDIsValid(mem->uuid)) {
+if (mem->uuid) {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 
 virUUIDFormat(mem->uuid, uuidstr);
-- 
2.26.2

Re: [PATCH] storage: Linstor support

[Andrea Bolognani]

On Mon, 2021-01-18 at 14:12 +0100, Peter Krempa wrote:
> On Fri, Jan 15, 2021 at 08:16:47 +0100, Rene Peinthor wrote:
> > +#define LINSTORCLI "linstor"
> 
> This should be defined via the build system once you detect where the
> program is located, to prevent PATH env variable from playing a role in
> which program is actually used.

Note that there seems to be an agreement on moving away from
performing build time checks for programs that are only used at
runtime, which I believe is the case here.

https://www.redhat.com/archives/libvir-list/2020-November/msg00650.html

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] storage: Linstor support

[Peter Krempa]

On Fri, Jan 15, 2021 at 08:16:47 +0100, Rene Peinthor wrote:
> Implement a LINSTOR backend storage driver.
> The Linstor client needs to be installed and it needs to be configured
> on the nodes used by the controller.
> 
> It supports most pool/vol commands, except for pool-build/pool-delete
> and provides a block device in RAW file mode.
> Linstor supports more than just DRBD so it would also be possible to have
> it provide LVM, ZFS or NVME volumes, but the common case will be to provide
> DRBD volumes in a cluster.
> 
> Sample pool XML:
> 
>   linstor
>   
> 
> libvirtgrp
>   
> 
> 
>  element must point to an already created LINSTOR
> resource-group, which is used to spawn resources/volumes.
>  attribute should be the local linstor node name,
> if missing it will try to get the hosts uname and use that instead.
> 
> Result volume XML sample:
> 
>   alpine12
>   libvirtgrp/alpine12
>   5368709120
>   5540028416
>   
> /dev/drbd1000
> 
>   
> 
> 
> Signed-off-by: Rene Peinthor 
> ---

Firstly I'd like you to split the patch into more granular commits as
it's customary in our project ...

>  docs/schemas/storagepool.rng  |  27 +
>  docs/storage.html.in  |  39 +
>  include/libvirt/libvirt-storage.h |   1 +
>  meson.build   |   6 +
>  meson_options.txt |   1 +
>  po/POTFILES.in|   1 +
>  src/conf/domain_conf.c|   1 +
>  src/conf/storage_conf.c   |  14 +-
>  src/conf/storage_conf.h   |   1 +
>  src/conf/virstorageobj.c  |   4 +-a

Conf changes are usually separate

>  src/storage/meson.build   |  25 +
>  src/storage/storage_backend.c |   6 +
>  src/storage/storage_backend_linstor.c | 803 ++
>  src/storage/storage_backend_linstor.h |  23 +
>  src/storage/storage_backend_linstor_priv.h|  53 ++
>  src/storage/storage_driver.c  |   1 +

Implementation should also be separate

>  src/test/test_driver.c|   1 +
>  tests/linstorjsondata/broken.json |   1 +
>  tests/linstorjsondata/resource-group.json |   1 +
>  .../linstorjsondata/resource-list-test2.json  | 332 
>  .../storage-pools-ssdpool.json|  72 ++
>  tests/linstorjsondata/storage-pools.json  | 192 +
>  tests/linstorjsondata/volume-def-list.json| 158 
>  .../volume-definition-test2.json  |   1 +
>  tests/meson.build |   6 +
>  tests/storagebackendlinstortest.c | 371 
>  .../storagepoolcapsschemadata/poolcaps-fs.xml |   7 +
>  .../poolcaps-full.xml |   7 +
>  tests/storagepoolxml2argvtest.c   |   1 +
>  tests/storagepoolxml2xmlin/pool-linstor.xml   |   8 +
>  tests/storagevolxml2xmlin/vol-linstor.xml |  10 +

Placing tests depends. Usually XML related tests go with the commits
implementing the parser/formatter or follow that commit.

Other tests should be added separately.

>  tools/virsh-pool.c|   3 +

This is adding the support for the new type so it goes where the type is
declared

>  32 files changed, 2175 insertions(+), 2 deletions(-)

[...]

> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
> index bd24b8b8d0..9b163e611d 100644
> --- a/docs/schemas/storagepool.rng
> +++ b/docs/schemas/storagepool.rng
> @@ -26,6 +26,7 @@
>  
>  
>  
> +
>
>  
>
> @@ -224,6 +225,21 @@
>  
>
>  
> +  
> +
> +  linstor
> +
> +
> +  
> +  
> +  
> +  
> +  
> +
> +  
> +
> +  
> +
>
>  
>
> @@ -463,6 +479,17 @@
>  
>
>  
> +  
> +
> +  
> +
> +
> +  
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/docs/storage.html.in b/docs/storage.html.in
> index b2cf343933..9130fbd180 100644
> --- a/docs/storage.html.in
> +++ b/docs/storage.html.in
> @@ -829,5 +829,44 @@
>  
>  Valid volume format types
>  The valid volume types are the same as for the directory pool.
> +
> +
> +LINSTOR pool
> +
> +  This provides a pool using the LINSTOR software-defined-storage.
> +  LINSTOR can provide block storage devices based on DRBD or basic
> +  LVM/ZFS volumes.
> +
> +
> +
> +  To use LINSTOR in libvirt, setup a working LINSTOR cluster, 
> documentation
> +  for that is in the LINSTOR Users-guide.

Could you link to it?

Also I'd like to ask you to provide a way to setup this storage on our
CI system so that we can compile-test the new driver.

We'd also like to know if you are willing to continue maintaining this
storage driver, so that it doesn't become abandonware right at commit
time.

...

[PATCH] storage: Linstor support

[Rene Peinthor]

Implement a LINSTOR backend storage driver.
The Linstor client needs to be installed and it needs to be configured
on the nodes used by the controller.

It supports most pool/vol commands, except for pool-build/pool-delete
and provides a block device in RAW file mode.
Linstor supports more than just DRBD so it would also be possible to have
it provide LVM, ZFS or NVME volumes, but the common case will be to provide
DRBD volumes in a cluster.

Sample pool XML:

  linstor
  

libvirtgrp
  


 element must point to an already created LINSTOR
resource-group, which is used to spawn resources/volumes.
 attribute should be the local linstor node name,
if missing it will try to get the hosts uname and use that instead.

Result volume XML sample:

  alpine12
  libvirtgrp/alpine12
  5368709120
  5540028416
  
/dev/drbd1000

  


Signed-off-by: Rene Peinthor 
---
 docs/schemas/storagepool.rng  |  27 +
 docs/storage.html.in  |  39 +
 include/libvirt/libvirt-storage.h |   1 +
 meson.build   |   6 +
 meson_options.txt |   1 +
 po/POTFILES.in|   1 +
 src/conf/domain_conf.c|   1 +
 src/conf/storage_conf.c   |  14 +-
 src/conf/storage_conf.h   |   1 +
 src/conf/virstorageobj.c  |   4 +-
 src/storage/meson.build   |  25 +
 src/storage/storage_backend.c |   6 +
 src/storage/storage_backend_linstor.c | 803 ++
 src/storage/storage_backend_linstor.h |  23 +
 src/storage/storage_backend_linstor_priv.h|  53 ++
 src/storage/storage_driver.c  |   1 +
 src/test/test_driver.c|   1 +
 tests/linstorjsondata/broken.json |   1 +
 tests/linstorjsondata/resource-group.json |   1 +
 .../linstorjsondata/resource-list-test2.json  | 332 
 .../storage-pools-ssdpool.json|  72 ++
 tests/linstorjsondata/storage-pools.json  | 192 +
 tests/linstorjsondata/volume-def-list.json| 158 
 .../volume-definition-test2.json  |   1 +
 tests/meson.build |   6 +
 tests/storagebackendlinstortest.c | 371 
 .../storagepoolcapsschemadata/poolcaps-fs.xml |   7 +
 .../poolcaps-full.xml |   7 +
 tests/storagepoolxml2argvtest.c   |   1 +
 tests/storagepoolxml2xmlin/pool-linstor.xml   |   8 +
 tests/storagevolxml2xmlin/vol-linstor.xml |  10 +
 tools/virsh-pool.c|   3 +
 32 files changed, 2175 insertions(+), 2 deletions(-)
 create mode 100644 src/storage/storage_backend_linstor.c
 create mode 100644 src/storage/storage_backend_linstor.h
 create mode 100644 src/storage/storage_backend_linstor_priv.h
 create mode 100644 tests/linstorjsondata/broken.json
 create mode 100644 tests/linstorjsondata/resource-group.json
 create mode 100644 tests/linstorjsondata/resource-list-test2.json
 create mode 100644 tests/linstorjsondata/storage-pools-ssdpool.json
 create mode 100644 tests/linstorjsondata/storage-pools.json
 create mode 100644 tests/linstorjsondata/volume-def-list.json
 create mode 100644 tests/linstorjsondata/volume-definition-test2.json
 create mode 100644 tests/storagebackendlinstortest.c
 create mode 100644 tests/storagepoolxml2xmlin/pool-linstor.xml
 create mode 100644 tests/storagevolxml2xmlin/vol-linstor.xml

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index bd24b8b8d0..9b163e611d 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -26,6 +26,7 @@
 
 
 
+
   
 
   
@@ -224,6 +225,21 @@
 
   
 
+  
+
+  linstor
+
+
+  
+  
+  
+  
+  
+
+  
+
+  
+
   
 
   
@@ -463,6 +479,17 @@
 
   
 
+  
+
+  
+
+
+  
+
+  
+
+  
+
   
 
   
diff --git a/docs/storage.html.in b/docs/storage.html.in
index b2cf343933..9130fbd180 100644
--- a/docs/storage.html.in
+++ b/docs/storage.html.in
@@ -829,5 +829,44 @@
 
 Valid volume format types
 The valid volume types are the same as for the directory pool.
+
+
+LINSTOR pool
+
+  This provides a pool using the LINSTOR software-defined-storage.
+  LINSTOR can provide block storage devices based on DRBD or basic
+  LVM/ZFS volumes.
+
+
+
+  To use LINSTOR in libvirt, setup a working LINSTOR cluster, documentation
+  for that is in the LINSTOR Users-guide.
+  And create a resource-group that will be used by libvirt, also make sure
+  the resource-group is setup in a way so that all nodes you want to use 
with libvirt
+  will create a resource. So either use diskless-on-remaining or make sure
+  replica-count is the same as you have nodes 

Re: [PATCH v2] spec: Add the man pages of split daemons

[Daniel P . Berrangé]

On Sat, Jan 16, 2021 at 03:16:18PM +0800, Han Han wrote:
> Fix the errors from commit a7cafa7bc2 when build RPMs from spec file:
> error: Installed (but unpackaged) file(s) found:
>/usr/share/man/man8/virtinterfaced.8.gz
>/usr/share/man/man8/virtlxcd.8.gz
>/usr/share/man/man8/virtnetworkd.8.gz
>/usr/share/man/man8/virtnodedevd.8.gz
>/usr/share/man/man8/virtnwfilterd.8.gz
>/usr/share/man/man8/virtproxyd.8.gz
>/usr/share/man/man8/virtqemud.8.gz
>/usr/share/man/man8/virtsecretd.8.gz
>/usr/share/man/man8/virtstoraged.8.gz
>/usr/share/man/man8/virtvboxd.8.gz
>/usr/share/man/man8/virtxend.8.gz
> 
> Signed-off-by: Han Han 
> ---
> Diff from v1: remove man page for virtvzd and virtbhyved
> ---
>  libvirt.spec.in | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index b5892987cf..9458a7a02f 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1581,6 +1581,16 @@ exit 0
>  %{_mandir}/man8/virtlogd.8*
>  %{_mandir}/man8/virtlockd.8*
>  %{_mandir}/man8/virtproxyd.8*
> +%{_mandir}/man8/virtxend.8*
> +%{_mandir}/man8/virtvboxd.8*
> +%{_mandir}/man8/virtstoraged.8*
> +%{_mandir}/man8/virtsecretd.8*
> +%{_mandir}/man8/virtqemud.8*
> +%{_mandir}/man8/virtnwfilterd.8*
> +%{_mandir}/man8/virtnodedevd.8*
> +%{_mandir}/man8/virtnetworkd.8*
> +%{_mandir}/man8/virtlxcd.8*
> +%{_mandir}/man8/virtinterfaced.8*
>  %{_mandir}/man7/virkey*.7*

This doesn't make any sense - those man pages are all listed already
against the individual sub-RPMs that contain the relevant daemon
binary.

$ grep man8/virt libvirt.spec.in 
%{_mandir}/man8/virtlogd.8*
%{_mandir}/man8/virtlockd.8*
%{_mandir}/man8/virtproxyd.8*
%{_mandir}/man8/virtinterfaced.8*
%{_mandir}/man8/virtnetworkd.8*
%{_mandir}/man8/virtnodedevd.8*
%{_mandir}/man8/virtnwfilterd.8*
%{_mandir}/man8/virtsecretd.8*
%{_mandir}/man8/virtstoraged.8*
%{_mandir}/man8/virtqemud.8*
%{_mandir}/man8/virtlxcd.8*
%{_mandir}/man8/virtxend.8*
%{_mandir}/man8/virtvboxd.8*
%{_mandir}/man8/virt-sanlock-cleanup.8*


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH] meson: Fix build with -Dtest_coverage=true

[Pavel Hrdina]

On Mon, Jan 18, 2021 at 09:23:34AM +0100, Jiri Denemark wrote:
> As can be seen in commit 8a62a1592ae00eab4eb153c02661e56b9d8d9032 (from
> autoconf era), the coverage flags have to be used also when linking
> objects. However, this was not reflected when we switched to meson.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/meson.build | 1 +
>  tests/meson.build   | 8 
>  tools/nss/meson.build   | 2 ++
>  tools/wireshark/src/meson.build | 3 +++
>  4 files changed, 14 insertions(+)
> 
> diff --git a/src/meson.build b/src/meson.build
> index 7c478219d6..980578d5d6 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -21,6 +21,7 @@ src_dep = declare_dependency(
>  + coverage_flags
>  + driver_modules_flags
>  + win32_link_flags
> ++ coverage_flags

You can see that it is already included few lines above.

>),
>  )
>  
> diff --git a/tests/meson.build b/tests/meson.build
> index f1d91ca50d..c65487f5c2 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -202,6 +202,9 @@ foreach mock : mock_libs
>libvirt_lib,
>mock.get('link_with', []),
>  ],
> +link_args: [
> +  coverage_flags,
> +],
>)
>  endforeach
>  
> @@ -218,6 +221,7 @@ executable(
>],
>link_args: [
>  libvirt_no_indirect,
> +coverage_flags
>],
>  )
>  
> @@ -566,6 +570,7 @@ foreach data : tests
>  ],
>  link_args: [
>libvirt_no_indirect,
> +  coverage_flags,
>  ],
>  link_with: [
>libvirt_lib,
> @@ -644,6 +649,9 @@ foreach data : helpers
>  link_with: [
>data['link_with'],
>  ],
> +link_args: [
> +  coverage_flags,
> +],
>  export_dynamic: true,
>)
>  endforeach

This should not be needed as well because coverage_flags is part of
tests_dep in "compile_args" which is the same as CFLAGS in automake.

The only place that uses COVERAGE_LDFLAGS is our fake SSH binary which
should be translated to link_args and we already do that.

If it has to be included when linking then we should remove the usage of
coverage_flags from the fake SSH and add it into tests_dep as link_args
in addition to compile_args.

> diff --git a/tools/nss/meson.build b/tools/nss/meson.build
> index cf3eec9b24..198936f3d4 100644
> --- a/tools/nss/meson.build
> +++ b/tools/nss/meson.build
> @@ -66,6 +66,7 @@ nss_libvirt_lib = shared_module(
>link_args: [
>  nss_libvirt_syms,
>  libvirt_export_dynamic,
> +coverage_flags,
>],
>link_whole: [
>  nss_libvirt_impl,
> @@ -81,6 +82,7 @@ nss_libvirt_guest_lib = shared_library(
>link_args: [
>  nss_libvirt_guest_syms,
>  libvirt_export_dynamic,
> +coverage_flags,
>],
>link_whole: [
>  nss_libvirt_guest_impl,
> diff --git a/tools/wireshark/src/meson.build b/tools/wireshark/src/meson.build
> index 49ccc9bb86..9b452dc5ca 100644
> --- a/tools/wireshark/src/meson.build
> +++ b/tools/wireshark/src/meson.build
> @@ -12,6 +12,9 @@ shared_library(
>  xdr_dep,
>  tools_dep,
>],
> +  link_args: [
> +coverage_flags
> +  ],
>install: true,
>install_dir: wireshark_plugindir,
>  )

The commit mentioned doesn't add COVERAGE_LDFLAGS into nss or wireshark
libs and checking our code before the meson switch we don't have it
there as well. It's not even in AM_LDFLAGS so looks like preexisting.

Pavel

> -- 
> 2.30.0
> 


signature.asc
Description: PGP signature

[libvirt PATCH] meson: Fix build with -Dtest_coverage=true

[Jiri Denemark]

As can be seen in commit 8a62a1592ae00eab4eb153c02661e56b9d8d9032 (from
autoconf era), the coverage flags have to be used also when linking
objects. However, this was not reflected when we switched to meson.

Signed-off-by: Jiri Denemark 
---
 src/meson.build | 1 +
 tests/meson.build   | 8 
 tools/nss/meson.build   | 2 ++
 tools/wireshark/src/meson.build | 3 +++
 4 files changed, 14 insertions(+)

diff --git a/src/meson.build b/src/meson.build
index 7c478219d6..980578d5d6 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -21,6 +21,7 @@ src_dep = declare_dependency(
 + coverage_flags
 + driver_modules_flags
 + win32_link_flags
++ coverage_flags
   ),
 )
 
diff --git a/tests/meson.build b/tests/meson.build
index f1d91ca50d..c65487f5c2 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -202,6 +202,9 @@ foreach mock : mock_libs
   libvirt_lib,
   mock.get('link_with', []),
 ],
+link_args: [
+  coverage_flags,
+],
   )
 endforeach
 
@@ -218,6 +221,7 @@ executable(
   ],
   link_args: [
 libvirt_no_indirect,
+coverage_flags
   ],
 )
 
@@ -566,6 +570,7 @@ foreach data : tests
 ],
 link_args: [
   libvirt_no_indirect,
+  coverage_flags,
 ],
 link_with: [
   libvirt_lib,
@@ -644,6 +649,9 @@ foreach data : helpers
 link_with: [
   data['link_with'],
 ],
+link_args: [
+  coverage_flags,
+],
 export_dynamic: true,
   )
 endforeach
diff --git a/tools/nss/meson.build b/tools/nss/meson.build
index cf3eec9b24..198936f3d4 100644
--- a/tools/nss/meson.build
+++ b/tools/nss/meson.build
@@ -66,6 +66,7 @@ nss_libvirt_lib = shared_module(
   link_args: [
 nss_libvirt_syms,
 libvirt_export_dynamic,
+coverage_flags,
   ],
   link_whole: [
 nss_libvirt_impl,
@@ -81,6 +82,7 @@ nss_libvirt_guest_lib = shared_library(
   link_args: [
 nss_libvirt_guest_syms,
 libvirt_export_dynamic,
+coverage_flags,
   ],
   link_whole: [
 nss_libvirt_guest_impl,
diff --git a/tools/wireshark/src/meson.build b/tools/wireshark/src/meson.build
index 49ccc9bb86..9b452dc5ca 100644
--- a/tools/wireshark/src/meson.build
+++ b/tools/wireshark/src/meson.build
@@ -12,6 +12,9 @@ shared_library(
 xdr_dep,
 tools_dep,
   ],
+  link_args: [
+coverage_flags
+  ],
   install: true,
   install_dir: wireshark_plugindir,
 )
-- 
2.30.0