[libvirt] [PATCH 7/8] virHostdevPrepareSCSIVHostDevices: Simplify logic

Michal Privoznik posted 8 patches 6 years, 7 months ago
[libvirt] [PATCH 7/8] virHostdevPrepareSCSIVHostDevices: Simplify logic
Posted by Michal Privoznik 6 years, 7 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/8] virHostdevPrepareSCSIVHostDevices: Simplify logic
Posted by Ján Tomko 6 years, 7 months ago
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list