- Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
an @obj, just return directly. This then allows the cleanup: label code
to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
This also simplifies some exit logic...
- In testNodeDeviceObjFindByName use an error: label to handle the failure
and don't do the ncaps++ within the VIR_STRDUP() source target index.
Only increment ncaps after success. Easier on eyes at error label too.
- In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/test/test_driver.c | 75 +++++++++++++++++++-------------------------------
1 file changed, 29 insertions(+), 46 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2db3f7d..3389edd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn,
const char *wwnn ATTRIBUTE_UNUSED,
const char *wwpn ATTRIBUTE_UNUSED)
{
- int ret = -1;
virNodeDeviceObjPtr obj = NULL;
virObjectEventPtr event = NULL;
@@ -4526,7 +4525,7 @@ testDestroyVport(testDriverPtr privconn,
if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) {
virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
_("no node device with matching name 'scsi_host12'"));
- goto cleanup;
+ return -1;
}
event = virNodeDeviceEventLifecycleNew("scsi_host12",
@@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn,
virNodeDeviceObjRemove(&privconn->devs, &obj);
- ret = 0;
-
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
testObjectEventQueue(privconn, event);
- return ret;
+ return 0;
}
@@ -5328,16 +5322,14 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
virNodeDevicePtr ret = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, name)))
- goto cleanup;
+ return NULL;
if ((ret = virGetNodeDevice(conn, name))) {
if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
virObjectUnref(ret);
}
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -5352,13 +5344,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
virCheckFlags(0, NULL);
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return NULL;
ret = virNodeDeviceDefFormat(obj->def);
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -5370,7 +5360,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
char *ret = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return NULL;
if (obj->def->parent) {
ignore_value(VIR_STRDUP(ret, obj->def->parent));
@@ -5379,9 +5369,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
"%s", _("no parent for this device"));
}
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -5393,19 +5381,15 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev)
virNodeDeviceObjPtr obj;
virNodeDevCapsDefPtr caps;
int ncaps = 0;
- int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return -1;
for (caps = obj->def->caps; caps; caps = caps->next)
++ncaps;
- ret = ncaps;
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
- return ret;
+ virNodeDeviceObjUnlock(obj);
+ return ncaps;
}
@@ -5416,26 +5400,25 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
virNodeDeviceObjPtr obj;
virNodeDevCapsDefPtr caps;
int ncaps = 0;
- int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return -1;
for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) {
- if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0)
- goto cleanup;
+ if (VIR_STRDUP(names[ncaps],
+ virNodeDevCapTypeToString(caps->data.type)) < 0)
+ goto error;
+ ncaps++;
}
- ret = ncaps;
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
- if (ret == -1) {
- --ncaps;
- while (--ncaps >= 0)
- VIR_FREE(names[ncaps]);
- }
- return ret;
+ virNodeDeviceObjUnlock(obj);
+ return ncaps;
+
+ error:
+ while (--ncaps >= 0)
+ VIR_FREE(names[ncaps]);
+ virNodeDeviceObjUnlock(obj);
+ return -1;
}
@@ -5584,13 +5567,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
virObjectEventPtr event = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto out;
+ return -1;
if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) == -1)
- goto out;
+ goto cleanup;
if (VIR_STRDUP(parent_name, obj->def->parent) < 0)
- goto out;
+ goto cleanup;
/* virNodeDeviceGetParentHost will cause the device object's lock to be
* taken, so we have to dup the parent's name and drop the lock
@@ -5603,7 +5586,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
if (virNodeDeviceObjGetParentHost(&driver->devs, obj->def,
EXISTING_DEVICE) < 0) {
obj = NULL;
- goto out;
+ goto cleanup;
}
event = virNodeDeviceEventLifecycleNew(dev->name,
@@ -5613,7 +5596,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
virNodeDeviceObjLock(obj);
virNodeDeviceObjRemove(&driver->devs, &obj);
- out:
+ cleanup:
if (obj)
virNodeDeviceObjUnlock(obj);
testObjectEventQueue(driver, event);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 25, 2017 at 15:56:58 -0400, John Ferlan wrote:
> - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
> an @obj, just return directly. This then allows the cleanup: label code
> to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
> This also simplifies some exit logic...
>
> - In testNodeDeviceObjFindByName use an error: label to handle the failure
> and don't do the ncaps++ within the VIR_STRDUP() source target index.
> Only increment ncaps after success. Easier on eyes at error label too.
>
> - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/test/test_driver.c | 75 +++++++++++++++++++-------------------------------
> 1 file changed, 29 insertions(+), 46 deletions(-)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 2db3f7d..3389edd 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn,
> const char *wwnn ATTRIBUTE_UNUSED,
> const char *wwpn ATTRIBUTE_UNUSED)
> {
> - int ret = -1;
> virNodeDeviceObjPtr obj = NULL;
> virObjectEventPtr event = NULL;
>
> @@ -4526,7 +4525,7 @@ testDestroyVport(testDriverPtr privconn,
> if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) {
> virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
> _("no node device with matching name 'scsi_host12'"));
> - goto cleanup;
> + return -1;
> }
>
> event = virNodeDeviceEventLifecycleNew("scsi_host12",
> @@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn,
>
> virNodeDeviceObjRemove(&privconn->devs, &obj);
A static analyzer may argue that 'obj' may be non-null in some cases at
this point ...
>
> - ret = 0;
> -
> - cleanup:
> - if (obj)
> - virNodeDeviceObjUnlock(obj);
So this would not unlock it.
> testObjectEventQueue(privconn, event);
> - return ret;
> + return 0;
> }
ACK to the rest
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/26/2017 03:05 AM, Peter Krempa wrote:
> On Thu, May 25, 2017 at 15:56:58 -0400, John Ferlan wrote:
>> - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
>> an @obj, just return directly. This then allows the cleanup: label code
>> to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
>> This also simplifies some exit logic...
>>
>> - In testNodeDeviceObjFindByName use an error: label to handle the failure
>> and don't do the ncaps++ within the VIR_STRDUP() source target index.
>> Only increment ncaps after success. Easier on eyes at error label too.
>>
>> - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/test/test_driver.c | 75 +++++++++++++++++++-------------------------------
>> 1 file changed, 29 insertions(+), 46 deletions(-)
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 2db3f7d..3389edd 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn,
>> const char *wwnn ATTRIBUTE_UNUSED,
>> const char *wwpn ATTRIBUTE_UNUSED)
>> {
>> - int ret = -1;
>> virNodeDeviceObjPtr obj = NULL;
>> virObjectEventPtr event = NULL;
>>
>> @@ -4526,7 +4525,7 @@ testDestroyVport(testDriverPtr privconn,
>> if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) {
>> virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
>> _("no node device with matching name 'scsi_host12'"));
>> - goto cleanup;
>> + return -1;
>> }
>>
>> event = virNodeDeviceEventLifecycleNew("scsi_host12",
>> @@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn,
>>
>> virNodeDeviceObjRemove(&privconn->devs, &obj);
>
> A static analyzer may argue that 'obj' may be non-null in some cases at
> this point ...
>
Not the one I've used... It seems it was smart enough to realize that
virNodeDeviceObjRemove will unlock *obj and then lock/unlock *obj each
time through it's loop and then set it to NULL if it matches something
on list. So either we return with obj=NULL or an unlocked obj
>>
>> - ret = 0;
>> -
>> - cleanup:
>> - if (obj)
>> - virNodeDeviceObjUnlock(obj);
>
> So this would not unlock it.
>
Or did I miss something more subtle? When originally wrote the code I
have to assume now I was paying less attention to what got called.
Tks -
John
>> testObjectEventQueue(privconn, event);
>> - return ret;
>> + return 0;
>> }
>
> ACK to the rest
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.