[libvirt] [PATCH 0/8] virhostdev: Couple of improvements
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
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@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
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@redhat.com> --- src/util/virhostdev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Michal Privoznik <mprivozn@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
On Tue, Jun 18, 2019 at 03:04:21PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virhostdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There's no need to translate virDomainHostdevDef-s into virPCIDevice-s with locked list of PCI devices. Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com> --- src/util/virhostdev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There's no need to translate virDomainHostdevDef-s into virPCIDevice-s with locked list of PCI devices. Signed-off-by: Michal Privoznik <mprivozn@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
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@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@redhat.com> Jano
There are couple of functions which get shorter after the treatment. Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com> --- src/util/virhostdev.c | 105 ++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 66 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@redhat.com> --- src/util/virhostdev.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@redhat.com> --- src/util/virhostdev.c | 41 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 25 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There are couple of functions which get shorter after the treatment. Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com> --- src/util/virhostdev.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik