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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
