8:04 a.m.
I'm working around this area and noticed a room for improvements.
This is basically a few patches that I have at the bottom if my branch
and they are independent of the feature I'm working on.
Michal Prívozník (8):
virHostdevGetPCIHostDeviceList: Add @pci a bit later
virHostdevGetPCIHostDeviceList: Use VIR_AUTOPTR for virPCIDevice
virHostdevPreparePCIDevices: Construct pcidevs list earlier
virHostdevReAttachPCIDevices: Construct pcidevs list earlier
virhostdev: Use VIR_AUTOUNREF more
virHostdevFindUSBDevice: Simplify flow a bit
virHostdevPrepareSCSIVHostDevices: Simplify logic
virhostdev: Use VIR_AUTOPTR more
src/util/virhostdev.c | 220 +++++++++++++++++-------------------------
1 file changed, 87 insertions(+), 133 deletions(-)
--
2.21.0
8:04 a.m.
New subject: [libvirt] [PATCH 1/8] virHostdevGetPCIHostDeviceList: Add @pci a bit later
This function is a good candidate for VIR_AUTOPTR() conversion.
But things conversion will be easier if we only add @pci device
onto @pcidevs list after it was all set up.
This is no functional change.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index fe176f35e4..9c63d0aebd 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -240,11 +240,6 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
virObjectUnref(pcidevs);
return NULL;
}
- if (virPCIDeviceListAdd(pcidevs, pci) < 0) {
- virPCIDeviceFree(pci);
- virObjectUnref(pcidevs);
- return NULL;
- }
virPCIDeviceSetManaged(pci, hostdev->managed);
@@ -254,6 +249,12 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN);
else
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM);
+
+ if (virPCIDeviceListAdd(pcidevs, pci) < 0) {
+ virPCIDeviceFree(pci);
+ virObjectUnref(pcidevs);
+ return NULL;
+ }
}
return pcidevs;
--
2.21.0
2:31 a.m.
New subject: [libvirt] [PATCH 1/8] virHostdevGetPCIHostDeviceList: Add @pci a bit later
On Tue, Jun 18, 2019 at 03:04:20PM +0200, Michal Privoznik wrote:
This function is a good candidate for VIR_AUTOPTR() conversion.
But things conversion will be easier if we only add @pci device
s/things/this/
onto @pcidevs list after it was all set up.
This is no functional change.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 2/8] virHostdevGetPCIHostDeviceList: Use VIR_AUTOPTR for virPCIDevice
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 9c63d0aebd..5935d926aa 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -227,7 +227,7 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
- virPCIDevicePtr pci;
+ VIR_AUTOPTR(virPCIDevice) pci = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
continue;
@@ -251,10 +251,10 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM);
if (virPCIDeviceListAdd(pcidevs, pci) < 0) {
- virPCIDeviceFree(pci);
virObjectUnref(pcidevs);
return NULL;
}
+ pci = NULL;
}
return pcidevs;
--
2.21.0
2:31 a.m.
New subject: [libvirt] [PATCH 2/8] virHostdevGetPCIHostDeviceList: Use VIR_AUTOPTR for virPCIDevice
On Tue, Jun 18, 2019 at 03:04:21PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 3/8] virHostdevPreparePCIDevices: Construct pcidevs list earlier
There's no need to translate virDomainHostdevDef-s into
virPCIDevice-s with locked list of PCI devices.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 5935d926aa..7b5ccf2daf 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -639,11 +639,11 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
if (!nhostdevs)
return 0;
- virObjectLock(mgr->activePCIHostdevs);
- virObjectLock(mgr->inactivePCIHostdevs);
-
if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs)))
- goto cleanup;
+ return -1;
+
+ virObjectLock(mgr->activePCIHostdevs);
+ virObjectLock(mgr->inactivePCIHostdevs);
/* Detaching devices from the host involves several steps; each
* of them is described at length below.
@@ -912,9 +912,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
}
cleanup:
- virObjectUnref(pcidevs);
virObjectUnlock(mgr->activePCIHostdevs);
virObjectUnlock(mgr->inactivePCIHostdevs);
+ virObjectUnref(pcidevs);
return ret;
}
--
2.21.0
2:31 a.m.
New subject: [libvirt] [PATCH 3/8] virHostdevPreparePCIDevices: Construct pcidevs list earlier
On Tue, Jun 18, 2019 at 03:04:22PM +0200, Michal Privoznik wrote:
There's no need to translate virDomainHostdevDef-s into
virPCIDevice-s with locked list of PCI devices.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 4/8] virHostdevReAttachPCIDevices: Construct pcidevs list earlier
There's no need to translate virDomainHostdevDef-s into
virPCIDevice-s with locked list of PCI devices.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 7b5ccf2daf..79a6ec86fe 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -963,16 +963,16 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
if (!nhostdevs)
return;
- virObjectLock(mgr->activePCIHostdevs);
- virObjectLock(mgr->inactivePCIHostdevs);
-
if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) {
VIR_ERROR(_("Failed to allocate PCI device list: %s"),
virGetLastErrorMessage());
virResetLastError();
- goto cleanup;
+ return;
}
+ virObjectLock(mgr->activePCIHostdevs);
+ virObjectLock(mgr->inactivePCIHostdevs);
+
/* Reattaching devices to the host involves several steps; each
* of them is described at length below */
--
2.21.0
2:32 a.m.
New subject: [libvirt] [PATCH 4/8] virHostdevReAttachPCIDevices: Construct pcidevs list earlier
On Tue, Jun 18, 2019 at 03:04:23PM +0200, Michal Privoznik wrote:
There's no need to translate virDomainHostdevDef-s into
virPCIDevice-s with locked list of PCI devices.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
util/virhostdev.c:1091:2: error: unused label 'cleanup' [-Werror,-Wunused-label]
cleanup:
^~~~~~~~
with that fixed:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 5/8] virhostdev: Use VIR_AUTOUNREF more
There are couple of functions which get shorter after the
treatment.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 105 ++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 66 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 79a6ec86fe..31ad287866 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -143,49 +143,49 @@ virHostdevManagerDispose(void *obj)
static virHostdevManagerPtr
virHostdevManagerNew(void)
{
- virHostdevManagerPtr hostdevMgr;
+ VIR_AUTOUNREF(virHostdevManagerPtr) hostdevMgr = NULL;
bool privileged = geteuid() == 0;
if (!(hostdevMgr = virObjectNew(virHostdevManagerClass)))
return NULL;
if (!(hostdevMgr->activePCIHostdevs = virPCIDeviceListNew()))
- goto error;
+ return NULL;
if (!(hostdevMgr->activeUSBHostdevs = virUSBDeviceListNew()))
- goto error;
+ return NULL;
if (!(hostdevMgr->inactivePCIHostdevs = virPCIDeviceListNew()))
- goto error;
+ return NULL;
if (!(hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew()))
- goto error;
+ return NULL;
if (!(hostdevMgr->activeSCSIVHostHostdevs = virSCSIVHostDeviceListNew()))
- goto error;
+ return NULL;
if (!(hostdevMgr->activeMediatedHostdevs = virMediatedDeviceListNew()))
- goto error;
+ return NULL;
if (privileged) {
if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) < 0)
- goto error;
+ return NULL;
if (virFileMakePath(hostdevMgr->stateDir) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("Failed to create state dir '%s'"),
hostdevMgr->stateDir);
- goto error;
+ return NULL;
}
} else {
VIR_AUTOFREE(char *) rundir = NULL;
mode_t old_umask;
if (!(rundir = virGetUserRuntimeDirectory()))
- goto error;
+ return NULL;
if (virAsprintf(&hostdevMgr->stateDir, "%s/hostdevmgr", rundir)
< 0)
- goto error;
+ return NULL;
old_umask = umask(077);
@@ -194,16 +194,12 @@ virHostdevManagerNew(void)
virReportError(VIR_ERR_OPERATION_FAILED,
_("Failed to create state dir '%s'"),
hostdevMgr->stateDir);
- goto error;
+ return NULL;
}
umask(old_umask);
}
- return hostdevMgr;
-
- error:
- virObjectUnref(hostdevMgr);
- return NULL;
+ VIR_RETURN_PTR(hostdevMgr);
}
virHostdevManagerPtr
@@ -218,7 +214,7 @@ virHostdevManagerGetDefault(void)
static virPCIDeviceListPtr
virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs)
{
- virPCIDeviceListPtr pcidevs;
+ VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL;
size_t i;
if (!(pcidevs = virPCIDeviceListNew()))
@@ -236,10 +232,8 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
pci = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
pcisrc->addr.slot, pcisrc->addr.function);
- if (!pci) {
- virObjectUnref(pcidevs);
+ if (!pci)
return NULL;
- }
virPCIDeviceSetManaged(pci, hostdev->managed);
@@ -250,14 +244,12 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
else
virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM);
- if (virPCIDeviceListAdd(pcidevs, pci) < 0) {
- virObjectUnref(pcidevs);
+ if (virPCIDeviceListAdd(pcidevs, pci) < 0)
return NULL;
- }
pci = NULL;
}
- return pcidevs;
+ VIR_RETURN_PTR(pcidevs);
}
@@ -630,7 +622,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
int nhostdevs,
unsigned int flags)
{
- virPCIDeviceListPtr pcidevs = NULL;
+ VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL;
int last_processed_hostdev_vf = -1;
size_t i;
int ret = -1;
@@ -914,7 +906,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
cleanup:
virObjectUnlock(mgr->activePCIHostdevs);
virObjectUnlock(mgr->inactivePCIHostdevs);
- virObjectUnref(pcidevs);
return ret;
}
@@ -957,7 +948,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
int nhostdevs,
const char *oldStateDir)
{
- virPCIDeviceListPtr pcidevs;
+ VIR_AUTOUNREF(virPCIDeviceListPtr) pcidevs = NULL;
size_t i;
if (!nhostdevs)
@@ -1088,8 +1079,6 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
virPCIDeviceGetName(actual));
}
- cleanup:
- virObjectUnref(pcidevs);
virObjectUnlock(mgr->activePCIHostdevs);
virObjectUnlock(mgr->inactivePCIHostdevs);
}
@@ -1407,7 +1396,7 @@ virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev,
* automatically found before.
*/
if (vendor) {
- virUSBDeviceListPtr devs;
+ VIR_AUTOUNREF(virUSBDeviceListPtr) devs = NULL;
rc = virUSBDeviceFindByVendor(vendor, product, NULL, mandatory, &devs);
if (rc < 0)
@@ -1417,7 +1406,6 @@ virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev,
*usb = virUSBDeviceListGet(devs, 0);
virUSBDeviceListSteal(devs, *usb);
}
- virObjectUnref(devs);
if (rc == 0) {
goto out;
@@ -1467,8 +1455,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
unsigned int flags)
{
size_t i;
- int ret = -1;
- virUSBDeviceListPtr list;
+ VIR_AUTOUNREF(virUSBDeviceListPtr) list = NULL;
virUSBDevicePtr tmp;
bool coldBoot = !!(flags & VIR_HOSTDEV_COLD_BOOT);
@@ -1481,7 +1468,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
* loop. See virHostdevPreparePCIDevices()
*/
if (!(list = virUSBDeviceListNew()))
- goto cleanup;
+ return -1;
/* Loop 1: build temporary list
*/
@@ -1501,11 +1488,11 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
required = false;
if (virHostdevFindUSBDevice(hostdev, required, &usb) < 0)
- goto cleanup;
+ return -1;
if (usb && virUSBDeviceListAdd(list, &usb) < 0) {
virUSBDeviceFree(usb);
- goto cleanup;
+ return -1;
}
}
@@ -1514,7 +1501,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
* wrong, perform rollback.
*/
if (virHostdevMarkUSBDevices(mgr, drv_name, dom_name, list) < 0)
- goto cleanup;
+ return -1;
/* Loop 2: Temporary list was successfully merged with
* driver list, so steal all items to avoid freeing them
@@ -1525,11 +1512,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
virUSBDeviceListSteal(list, tmp);
}
- ret = 0;
-
- cleanup:
- virObjectUnref(list);
- return ret;
+ return 0;
}
static int
@@ -1569,7 +1552,7 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr,
{
size_t i, j;
int count;
- virSCSIDeviceListPtr list;
+ VIR_AUTOUNREF(virSCSIDeviceListPtr) list = NULL;
virSCSIDevicePtr tmp;
if (!nhostdevs)
@@ -1581,7 +1564,7 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr,
* loop. See virHostdevPreparePCIDevices()
*/
if (!(list = virSCSIDeviceListNew()))
- goto cleanup;
+ return -1;
/* Loop 1: build temporary list */
for (i = 0; i < nhostdevs; i++) {
@@ -1595,7 +1578,7 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr,
continue; /* Not supported for iSCSI */
} else {
if (virHostdevPrepareSCSIHostDevices(hostdev, scsisrc, list) < 0)
- goto cleanup;
+ return -1;
}
}
@@ -1646,7 +1629,6 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr,
virSCSIDeviceListSteal(list, tmp);
}
- virObjectUnref(list);
return 0;
error:
@@ -1655,8 +1637,6 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr,
virSCSIDeviceListSteal(mgr->activeSCSIHostdevs, tmp);
}
virObjectUnlock(mgr->activeSCSIHostdevs);
- cleanup:
- virObjectUnref(list);
return -1;
}
@@ -1669,7 +1649,7 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
{
size_t i, j;
int count;
- virSCSIVHostDeviceListPtr list;
+ VIR_AUTOUNREF(virSCSIVHostDeviceListPtr) list = NULL;
virSCSIVHostDevicePtr host, tmp;
if (!nhostdevs)
@@ -1681,7 +1661,7 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
* loop. See virHostdevPreparePCIDevices()
*/
if (!(list = virSCSIVHostDeviceListNew()))
- goto cleanup;
+ return -1;
/* Loop 1: build temporary list */
for (i = 0; i < nhostdevs; i++) {
@@ -1696,11 +1676,11 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
continue; /* Not supported */
if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn)))
- goto cleanup;
+ return -1;
if (virSCSIVHostDeviceListAdd(list, host) < 0) {
virSCSIVHostDeviceFree(host);
- goto cleanup;
+ return -1;
}
}
@@ -1743,7 +1723,6 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
virSCSIVHostDeviceListSteal(list, tmp);
}
- virObjectUnref(list);
return 0;
error:
for (j = 0; j < i; j++) {
@@ -1751,8 +1730,6 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
virSCSIVHostDeviceListSteal(mgr->activeSCSIVHostHostdevs, tmp);
}
virObjectUnlock(mgr->activeSCSIVHostHostdevs);
- cleanup:
- virObjectUnref(list);
return -1;
}
@@ -1765,8 +1742,7 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
int nhostdevs)
{
size_t i;
- int ret = -1;
- virMediatedDeviceListPtr list;
+ VIR_AUTOUNREF(virMediatedDeviceListPtr) list = NULL;
if (!nhostdevs)
return 0;
@@ -1776,7 +1752,7 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
* A device is appended to the driver list after a series of preparations.
*/
if (!(list = virMediatedDeviceListNew()))
- goto cleanup;
+ return -1;
/* Loop 1: Build a temporary list of ALL mediated devices. */
for (i = 0; i < nhostdevs; i++) {
@@ -1788,11 +1764,11 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
continue;
if (!(mdev = virMediatedDeviceNew(src->uuidstr, src->model)))
- goto cleanup;
+ return -1;
if (virMediatedDeviceListAdd(list, &mdev) < 0) {
virMediatedDeviceFree(mdev);
- goto cleanup;
+ return -1;
}
}
@@ -1801,7 +1777,7 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
*/
if (virMediatedDeviceListMarkDevices(mgr->activeMediatedHostdevs,
list, drv_name, dom_name) < 0)
- goto cleanup;
+ return -1;
/* Loop 2: Temporary list was successfully merged with
* driver list, so steal all items to avoid freeing them
@@ -1812,10 +1788,7 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
virMediatedDeviceListSteal(list, tmp);
}
- ret = 0;
- cleanup:
- virObjectUnref(list);
- return ret;
+ return 0;
}
void
--
2.21.0
2:32 a.m.
New subject: [libvirt] [PATCH 5/8] virhostdev: Use VIR_AUTOUNREF more
On Tue, Jun 18, 2019 at 03:04:24PM +0200, Michal Privoznik wrote:
There are couple of functions which get shorter after the
treatment.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 105 ++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 66 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 6/8] virHostdevFindUSBDevice: Simplify flow a bit
When looking up a USB device by vendor the
virUSBDeviceFindByVendor() is used. The function returns number
of items found. But the logic in caller to process it is
needlessly complicated.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 31ad287866..9817c6571b 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1399,15 +1399,9 @@ virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev,
VIR_AUTOUNREF(virUSBDeviceListPtr) devs = NULL;
rc = virUSBDeviceFindByVendor(vendor, product, NULL, mandatory, &devs);
- if (rc < 0)
+ if (rc < 0) {
return -1;
-
- if (rc == 1) {
- *usb = virUSBDeviceListGet(devs, 0);
- virUSBDeviceListSteal(devs, *usb);
- }
-
- if (rc == 0) {
+ } else if (rc == 0) {
goto out;
} else if (rc > 1) {
if (autoAddress) {
@@ -1424,6 +1418,9 @@ virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev,
return -1;
}
+ *usb = virUSBDeviceListGet(devs, 0);
+ virUSBDeviceListSteal(devs, *usb);
+
usbsrc->bus = virUSBDeviceGetBus(*usb);
usbsrc->device = virUSBDeviceGetDevno(*usb);
usbsrc->autoAddress = true;
--
2.21.0
2:33 a.m.
New subject: [libvirt] [PATCH 6/8] virHostdevFindUSBDevice: Simplify flow a bit
On Tue, Jun 18, 2019 at 03:04:25PM +0200, Michal Privoznik wrote:
When looking up a USB device by vendor the
virUSBDeviceFindByVendor() is used. The function returns number
of items found. But the logic in caller to process it is
needlessly complicated.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 7/8] virHostdevPrepareSCSIVHostDevices: Simplify logic
Couple of things happening in this patch:
1) We can mark the device we're adding onto active list as used
way before - when adding it onto temporary list.
2) When actually moving device from a temporary helper list onto
the list of active devices we check if the device isn't
already there. The same check is performed by
virSCSIVHostDeviceListAdd() later. Drop this duplicity.
3) The 'error' label is renamed to 'rollback' to reflect what it
is actually doing. While in the rest of the code we don't
allow random label names, this source file is different.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 41 ++++++++++++++++-------------------------
1 file changed, 16 insertions(+), 25 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 9817c6571b..fe2d774857 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1644,10 +1644,9 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
virDomainHostdevDefPtr *hostdevs,
int nhostdevs)
{
- size_t i, j;
- int count;
VIR_AUTOUNREF(virSCSIVHostDeviceListPtr) list = NULL;
- virSCSIVHostDevicePtr host, tmp;
+ virSCSIVHostDevicePtr tmp;
+ size_t i, j;
if (!nhostdevs)
return 0;
@@ -1664,6 +1663,7 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
virDomainHostdevSubsysSCSIVHostPtr hostsrc =
&hostdev->source.subsys.u.scsi_host;
+ VIR_AUTOPTR(virSCSIVHostDevice) host = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST)
@@ -1675,10 +1675,12 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn)))
return -1;
- if (virSCSIVHostDeviceListAdd(list, host) < 0) {
- virSCSIVHostDeviceFree(host);
+ if (virSCSIVHostDeviceSetUsedBy(host, drv_name, dom_name) < 0)
return -1;
- }
+
+ if (virSCSIVHostDeviceListAdd(list, host) < 0)
+ return -1;
+ host = NULL;
}
/* Loop 2: Mark devices in temporary list as used by @name
@@ -1686,27 +1688,15 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
* wrong, perform rollback.
*/
virObjectLock(mgr->activeSCSIVHostHostdevs);
- count = virSCSIVHostDeviceListCount(list);
- for (i = 0; i < count; i++) {
- host = virSCSIVHostDeviceListGet(list, i);
- if ((tmp = virSCSIVHostDeviceListFind(mgr->activeSCSIVHostHostdevs,
- host))) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI_host device %s is already in use by "
- "another domain"),
- virSCSIVHostDeviceGetName(tmp));
- goto error;
- } else {
- if (virSCSIVHostDeviceSetUsedBy(host, drv_name, dom_name) < 0)
- goto error;
+ for (i = 0; i < virSCSIVHostDeviceListCount(list); i++) {
+ tmp = virSCSIVHostDeviceListGet(list, i);
- VIR_DEBUG("Adding %s to activeSCSIVHostHostdevs",
- virSCSIVHostDeviceGetName(host));
+ VIR_DEBUG("Adding %s to activeSCSIVHostHostdevs",
+ virSCSIVHostDeviceGetName(tmp));
- if (virSCSIVHostDeviceListAdd(mgr->activeSCSIVHostHostdevs, host) < 0)
- goto error;
- }
+ if (virSCSIVHostDeviceListAdd(mgr->activeSCSIVHostHostdevs, tmp) < 0)
+ goto rollback;
}
virObjectUnlock(mgr->activeSCSIVHostHostdevs);
@@ -1721,7 +1711,8 @@ virHostdevPrepareSCSIVHostDevices(virHostdevManagerPtr mgr,
}
return 0;
- error:
+
+ rollback:
for (j = 0; j < i; j++) {
tmp = virSCSIVHostDeviceListGet(list, i);
virSCSIVHostDeviceListSteal(mgr->activeSCSIVHostHostdevs, tmp);
--
2.21.0
2:33 a.m.
New subject: [libvirt] [PATCH 7/8] virHostdevPrepareSCSIVHostDevices: Simplify logic
On Tue, Jun 18, 2019 at 03:04:26PM +0200, Michal Privoznik wrote:
Couple of things happening in this patch:
1) We can mark the device we're adding onto active list as used
way before - when adding it onto temporary list.
2) When actually moving device from a temporary helper list onto
the list of active devices we check if the device isn't
already there. The same check is performed by
virSCSIVHostDeviceListAdd() later. Drop this duplicity.
3) The 'error' label is renamed to 'rollback' to reflect what it
is actually doing. While in the rest of the code we don't
allow random label names, this source file is different.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 41 ++++++++++++++++-------------------------
1 file changed, 16 insertions(+), 25 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:04 a.m.
New subject: [libvirt] [PATCH 8/8] virhostdev: Use VIR_AUTOPTR more
There are couple of functions which get shorter after the
treatment.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index fe2d774857..271a654a28 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1161,7 +1161,7 @@ virHostdevUpdateActiveUSBDevices(virHostdevManagerPtr mgr,
virObjectLock(mgr->activeUSBHostdevs);
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevSubsysUSBPtr usbsrc;
- virUSBDevicePtr usb = NULL;
+ VIR_AUTOPTR(virUSBDevice) usb = NULL;
hostdev = hostdevs[i];
usbsrc = &hostdev->source.subsys.u.usb;
@@ -1178,10 +1178,9 @@ virHostdevUpdateActiveUSBDevices(virHostdevManagerPtr mgr,
virUSBDeviceSetUsedBy(usb, drv_name, dom_name);
- if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, &usb) < 0) {
- virUSBDeviceFree(usb);
+ if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, &usb) < 0)
goto cleanup;
- }
+ usb = NULL;
}
ret = 0;
cleanup:
@@ -1197,7 +1196,7 @@ virHostdevUpdateActiveSCSIHostDevices(virHostdevManagerPtr mgr,
const char *dom_name)
{
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
- virSCSIDevicePtr scsi = NULL;
+ VIR_AUTOPTR(virSCSIDevice) scsi = NULL;
virSCSIDevicePtr tmp = NULL;
if (!(scsi = virSCSIDeviceNew(NULL,
@@ -1207,17 +1206,13 @@ virHostdevUpdateActiveSCSIHostDevices(virHostdevManagerPtr mgr,
return -1;
if ((tmp = virSCSIDeviceListFind(mgr->activeSCSIHostdevs, scsi))) {
- if (virSCSIDeviceSetUsedBy(tmp, drv_name, dom_name) < 0) {
- virSCSIDeviceFree(scsi);
+ if (virSCSIDeviceSetUsedBy(tmp, drv_name, dom_name) < 0)
return -1;
- }
- virSCSIDeviceFree(scsi);
} else {
if (virSCSIDeviceSetUsedBy(scsi, drv_name, dom_name) < 0 ||
- virSCSIDeviceListAdd(mgr->activeSCSIHostdevs, scsi) < 0) {
- virSCSIDeviceFree(scsi);
+ virSCSIDeviceListAdd(mgr->activeSCSIHostdevs, scsi) < 0)
return -1;
- }
+ scsi = NULL;
}
return 0;
}
@@ -1472,7 +1467,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
bool required = true;
- virUSBDevicePtr usb;
+ VIR_AUTOPTR(virUSBDevice) usb = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
continue;
@@ -1487,10 +1482,9 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
if (virHostdevFindUSBDevice(hostdev, required, &usb) < 0)
return -1;
- if (usb && virUSBDeviceListAdd(list, &usb) < 0) {
- virUSBDeviceFree(usb);
+ if (usb && virUSBDeviceListAdd(list, &usb) < 0)
return -1;
- }
+ usb = NULL;
}
/* Mark devices in temporary list as used by @dom_name
@@ -1518,7 +1512,7 @@ virHostdevPrepareSCSIHostDevices(virDomainHostdevDefPtr hostdev,
virSCSIDeviceListPtr list)
{
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
- virSCSIDevicePtr scsi;
+ VIR_AUTOPTR(virSCSIDevice) scsi = NULL;
if (hostdev->managed) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -1532,10 +1526,9 @@ virHostdevPrepareSCSIHostDevices(virDomainHostdevDefPtr hostdev,
hostdev->readonly, hostdev->shareable)))
return -1;
- if (virSCSIDeviceListAdd(list, scsi) < 0) {
- virSCSIDeviceFree(scsi);
+ if (virSCSIDeviceListAdd(list, scsi) < 0)
return -1;
- }
+ scsi = NULL;
return 0;
}
@@ -1746,7 +1739,7 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
virDomainHostdevSubsysMediatedDevPtr src =
&hostdev->source.subsys.u.mdev;
- virMediatedDevicePtr mdev;
+ VIR_AUTOPTR(virMediatedDevice) mdev = NULL;
if (!virHostdevIsMdevDevice(hostdev))
continue;
@@ -1754,10 +1747,9 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,
if (!(mdev = virMediatedDeviceNew(src->uuidstr, src->model)))
return -1;
- if (virMediatedDeviceListAdd(list, &mdev) < 0) {
- virMediatedDeviceFree(mdev);
+ if (virMediatedDeviceListAdd(list, &mdev) < 0)
return -1;
- }
+ mdev = NULL;
}
/* Mark the devices in the list as used by @drv_name-@dom_name and copy the
--
2.21.0
3:27 a.m.
New subject: [libvirt] [PATCH 8/8] virhostdev: Use VIR_AUTOPTR more
On Tue, Jun 18, 2019 at 03:04:27PM +0200, Michal Privoznik wrote:
There are couple of functions which get shorter after the
treatment.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virhostdev.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2012
days inactive
2014
days old
16 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik