This function is calling public APIs (virNodeDeviceLookupByName
etc.). That requires the driver lock to be unlocked and locked
again. If we, however, replace the public APIs calls with the
internal calls (that public APIs call anyway), we can drop the
lock/unlock exercise.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/test/test_driver.c | 59 ++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5fef3f10b..8495443db 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5626,17 +5626,16 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
 }
 
 
-static virNodeDeviceDefPtr
+static virNodeDeviceObjPtr
 testNodeDeviceMockCreateVport(virConnectPtr conn,
                               const char *wwnn,
                               const char *wwpn)
 {
     testDriverPtr driver = conn->privateData;
-    virNodeDevicePtr devcpy = NULL;
     char *xml = NULL;
     virNodeDeviceDefPtr def = NULL;
     virNodeDevCapsDefPtr caps;
-    virNodeDeviceObjPtr obj = NULL;
+    virNodeDeviceObjPtr obj = NULL, objcopy = NULL;
     virObjectEventPtr event = NULL;
 
     /* In the real code, we'd call virVHBAManageVport which would take the
@@ -5648,9 +5647,15 @@ testNodeDeviceMockCreateVport(virConnectPtr conn,
      * using the scsi_host11 definition, changing the name and the
      * scsi_host capability fields before calling virNodeDeviceAssignDef
      * to add the def to the node device objects list. */
-    if (!(devcpy = virNodeDeviceLookupByName(conn, "scsi_host11")) ||
-        !(xml = virNodeDeviceGetXMLDesc(devcpy, 0)) ||
-        !(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
+    if (!(objcopy = virNodeDeviceFindByName(&driver->devs, "scsi_host11")))
+        goto cleanup;
+
+    xml = virNodeDeviceDefFormat(objcopy->def);
+    virNodeDeviceObjUnlock(objcopy);
+    if (!xml)
+        goto cleanup;
+
+    if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
         goto cleanup;
 
     VIR_FREE(def->name);
@@ -5684,23 +5689,17 @@ testNodeDeviceMockCreateVport(virConnectPtr conn,
 
     if (!(obj = virNodeDeviceAssignDef(&driver->devs, def)))
         goto cleanup;
-    virNodeDeviceObjUnlock(obj);
+    def = NULL;
 
-    event = virNodeDeviceEventLifecycleNew(def->name,
+    event = virNodeDeviceEventLifecycleNew(obj->def->name,
                                            VIR_NODE_DEVICE_EVENT_CREATED,
                                            0);
     testObjectEventQueue(driver, event);
 
  cleanup:
     VIR_FREE(xml);
-    if (!obj) {
-        virNodeDeviceDefFree(def);
-        def = NULL;
-    }
-    if (devcpy)
-        virNodeDeviceFree(devcpy);
-
-    return def;
+    virNodeDeviceDefFree(def);
+    return obj;
 }
 
 
@@ -5712,8 +5711,8 @@ testNodeDeviceCreateXML(virConnectPtr conn,
     testDriverPtr driver = conn->privateData;
     virNodeDeviceDefPtr def = NULL;
     char *wwnn = NULL, *wwpn = NULL;
-    virNodeDevicePtr dev = NULL;
-    virNodeDeviceDefPtr newdef = NULL;
+    virNodeDevicePtr dev = NULL, ret = NULL;
+    virNodeDeviceObjPtr obj = NULL;
 
     virCheckFlags(0, NULL);
 
@@ -5739,20 +5738,28 @@ testNodeDeviceCreateXML(virConnectPtr conn,
      * mocking udev. The mock routine will copy an existing vHBA and
      * rename a few fields to mock that. So in order to allow that to
      * work properly, we need to drop our lock */
-    testDriverUnlock(driver);
-    if ((newdef = testNodeDeviceMockCreateVport(conn, wwnn, wwpn))) {
-        if ((dev = virNodeDeviceLookupByName(conn, newdef->name)))
-            ignore_value(VIR_STRDUP(dev->parent, def->parent));
-    }
-    testDriverLock(driver);
-    newdef = NULL;
+    if (!(obj = testNodeDeviceMockCreateVport(conn, wwnn, wwpn)))
+        goto cleanup;
+
+    if (!(dev = virGetNodeDevice(conn, obj->def->name)))
+        goto cleanup;
+
+    VIR_FREE(dev->parent);
+    if (VIR_STRDUP(dev->parent, def->parent) < 0)
+        goto cleanup;
+
+    ret = dev;
+    dev = NULL;
 
  cleanup:
+    if (obj)
+        virNodeDeviceObjUnlock(obj);
     testDriverUnlock(driver);
     virNodeDeviceDefFree(def);
+    virObjectUnref(dev);
     VIR_FREE(wwnn);
     VIR_FREE(wwpn);
-    return dev;
+    return ret;
 }
 
 static int
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/28/2017 07:12 AM, Michal Privoznik wrote:
> This function is calling public APIs (virNodeDeviceLookupByName
> etc.). That requires the driver lock to be unlocked and locked
> again. If we, however, replace the public APIs calls with the
> internal calls (that public APIs call anyway), we can drop the
> lock/unlock exercise.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/test/test_driver.c | 59 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 5fef3f10b..8495443db 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -5626,17 +5626,16 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
>  }
>  
>  
> -static virNodeDeviceDefPtr
> +static virNodeDeviceObjPtr
>  testNodeDeviceMockCreateVport(virConnectPtr conn,

Conceptually this could take the driver instead of conn since all conn
is used for is to get driver. I see testObjectEventQueue does this...

>                                const char *wwnn,
>                                const char *wwpn)
>  {
>      testDriverPtr driver = conn->privateData;
> -    virNodeDevicePtr devcpy = NULL;
>      char *xml = NULL;
>      virNodeDeviceDefPtr def = NULL;
>      virNodeDevCapsDefPtr caps;
> -    virNodeDeviceObjPtr obj = NULL;
> +    virNodeDeviceObjPtr obj = NULL, objcopy = NULL;

There are those that prefer separate lines for each, IDC, but I usually
just capitulate to keep the peace.

>      virObjectEventPtr event = NULL;
>  
>      /* In the real code, we'd call virVHBAManageVport which would take the
> @@ -5648,9 +5647,15 @@ testNodeDeviceMockCreateVport(virConnectPtr conn,
>       * using the scsi_host11 definition, changing the name and the
>       * scsi_host capability fields before calling virNodeDeviceAssignDef
>       * to add the def to the node device objects list. */
> -    if (!(devcpy = virNodeDeviceLookupByName(conn, "scsi_host11")) ||
> -        !(xml = virNodeDeviceGetXMLDesc(devcpy, 0)) ||
> -        !(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
> +    if (!(objcopy = virNodeDeviceFindByName(&driver->devs, "scsi_host11")))
> +        goto cleanup;
> +
> +    xml = virNodeDeviceDefFormat(objcopy->def);
> +    virNodeDeviceObjUnlock(objcopy);
> +    if (!xml)
> +        goto cleanup;
> +
> +    if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
>          goto cleanup;
>  
>      VIR_FREE(def->name);
> @@ -5684,23 +5689,17 @@ testNodeDeviceMockCreateVport(virConnectPtr conn,
>  
>      if (!(obj = virNodeDeviceAssignDef(&driver->devs, def)))
>          goto cleanup;
> -    virNodeDeviceObjUnlock(obj);
> +    def = NULL;
>  
> -    event = virNodeDeviceEventLifecycleNew(def->name,
> +    event = virNodeDeviceEventLifecycleNew(obj->def->name,
>                                             VIR_NODE_DEVICE_EVENT_CREATED,
>                                             0);
>      testObjectEventQueue(driver, event);
>  
>   cleanup:
>      VIR_FREE(xml);
> -    if (!obj) {
> -        virNodeDeviceDefFree(def);
> -        def = NULL;
> -    }
> -    if (devcpy)
> -        virNodeDeviceFree(devcpy);
> -
> -    return def;
> +    virNodeDeviceDefFree(def);
> +    return obj;
>  }
>  
>  
> @@ -5712,8 +5711,8 @@ testNodeDeviceCreateXML(virConnectPtr conn,
>      testDriverPtr driver = conn->privateData;
>      virNodeDeviceDefPtr def = NULL;
>      char *wwnn = NULL, *wwpn = NULL;
> -    virNodeDevicePtr dev = NULL;
> -    virNodeDeviceDefPtr newdef = NULL;
> +    virNodeDevicePtr dev = NULL, ret = NULL;

ditto.

> +    virNodeDeviceObjPtr obj = NULL;
>  
>      virCheckFlags(0, NULL);
>  
> @@ -5739,20 +5738,28 @@ testNodeDeviceCreateXML(virConnectPtr conn,
>       * mocking udev. The mock routine will copy an existing vHBA and
>       * rename a few fields to mock that. So in order to allow that to
>       * work properly, we need to drop our lock */
> -    testDriverUnlock(driver);
> -    if ((newdef = testNodeDeviceMockCreateVport(conn, wwnn, wwpn))) {
> -        if ((dev = virNodeDeviceLookupByName(conn, newdef->name)))
> -            ignore_value(VIR_STRDUP(dev->parent, def->parent));
> -    }
> -    testDriverLock(driver);
> -    newdef = NULL;
> +    if (!(obj = testNodeDeviceMockCreateVport(conn, wwnn, wwpn)))
> +        goto cleanup;
> +
> +    if (!(dev = virGetNodeDevice(conn, obj->def->name)))
> +        goto cleanup;
> +
> +    VIR_FREE(dev->parent);
> +    if (VIR_STRDUP(dev->parent, def->parent) < 0)
> +        goto cleanup;

FWIW: The whole reason for the VIR_STRDUP is because virGetNodeDevice
does not fill in dev->parent, so the VIR_FREE() really is unnecessary.

ACK (and safe) for what's here... Your call on the param change and the
multiple defs on the same line.

John

> +
> +    ret = dev;
> +    dev = NULL;
>  
>   cleanup:
> +    if (obj)
> +        virNodeDeviceObjUnlock(obj);
>      testDriverUnlock(driver);
>      virNodeDeviceDefFree(def);
> +    virObjectUnref(dev);
>      VIR_FREE(wwnn);
>      VIR_FREE(wwpn);
> -    return dev;
> +    return ret;
>  }
>  
>  static int
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/28/2017 03:24 PM, John Ferlan wrote:
> 
> 
> On 02/28/2017 07:12 AM, Michal Privoznik wrote:
>> This function is calling public APIs (virNodeDeviceLookupByName
>> etc.). That requires the driver lock to be unlocked and locked
>> again. If we, however, replace the public APIs calls with the
>> internal calls (that public APIs call anyway), we can drop the
>> lock/unlock exercise.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/test/test_driver.c | 59 ++++++++++++++++++++++++++++----------------------
>>  1 file changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 5fef3f10b..8495443db 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -5626,17 +5626,16 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
>>  }
>>  
>>  
>> -static virNodeDeviceDefPtr
>> +static virNodeDeviceObjPtr
>>  testNodeDeviceMockCreateVport(virConnectPtr conn,
> 
> Conceptually this could take the driver instead of conn since all conn
> is used for is to get driver. I see testObjectEventQueue does this...

Ah, good point. I'll fix that before pushing.

> 
>>                                const char *wwnn,
>>                                const char *wwpn)
>>  {
>>      testDriverPtr driver = conn->privateData;
>> -    virNodeDevicePtr devcpy = NULL;
>>      char *xml = NULL;
>>      virNodeDeviceDefPtr def = NULL;
>>      virNodeDevCapsDefPtr caps;
>> -    virNodeDeviceObjPtr obj = NULL;
>> +    virNodeDeviceObjPtr obj = NULL, objcopy = NULL;
> 
> There are those that prefer separate lines for each, IDC, but I usually
> just capitulate to keep the peace.

I don't really understand why. Multiple variables per line is just fine.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list