- 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 e323619..e91dfa3 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4509,7 +4509,6 @@ testDestroyVport(testDriverPtr privconn,
const char *wwnn ATTRIBUTE_UNUSED,
const char *wwpn ATTRIBUTE_UNUSED)
{
- int ret = -1;
virNodeDeviceObjPtr obj = NULL;
virObjectEventPtr event = NULL;
@@ -4523,7 +4522,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",
@@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn,
virNodeDeviceObjFree(obj);
obj = NULL;
- ret = 0;
-
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
testObjectEventQueue(privconn, event);
- return ret;
+ return 0;
}
@@ -5328,7 +5322,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
virNodeDevicePtr ret = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, name)))
- goto cleanup;
+ return NULL;
def = virNodeDeviceObjGetDef(obj);
if ((ret = virGetNodeDevice(conn, name))) {
@@ -5338,9 +5332,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
}
}
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -5355,13 +5347,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
virCheckFlags(0, NULL);
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return NULL;
ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj));
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -5374,7 +5364,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
char *ret = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return NULL;
def = virNodeDeviceObjGetDef(obj);
if (def->parent) {
@@ -5384,9 +5374,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
"%s", _("no parent for this device"));
}
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -5399,20 +5387,16 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev)
virNodeDeviceDefPtr def;
virNodeDevCapsDefPtr caps;
int ncaps = 0;
- int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return -1;
def = virNodeDeviceObjGetDef(obj);
for (caps = def->caps; caps; caps = caps->next)
++ncaps;
- ret = ncaps;
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
- return ret;
+ virNodeDeviceObjUnlock(obj);
+ return ncaps;
}
@@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
virNodeDeviceDefPtr def;
virNodeDevCapsDefPtr caps;
int ncaps = 0;
- int ret = -1;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto cleanup;
+ return -1;
def = virNodeDeviceObjGetDef(obj);
for (caps = 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;
}
@@ -5598,14 +5581,14 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
virObjectEventPtr event = NULL;
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
- goto out;
+ return -1;
def = virNodeDeviceObjGetDef(obj);
if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1)
- goto out;
+ goto cleanup;
if (VIR_STRDUP(parent_name, 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
@@ -5618,7 +5601,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
if (virNodeDeviceObjGetParentHost(&driver->devs, def,
EXISTING_DEVICE) < 0) {
obj = NULL;
- goto out;
+ goto cleanup;
}
event = virNodeDeviceEventLifecycleNew(dev->name,
@@ -5630,7 +5613,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
virNodeDeviceObjFree(obj);
obj = NULL;
- 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 Sat, Jun 03, 2017 at 09:11:52AM -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>
[..]
> event = virNodeDeviceEventLifecycleNew("scsi_host12",
> @@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn,
> virNodeDeviceObjFree(obj);
> obj = NULL;
You can drop ^this then since you don't need to clear it anymore.
Just a minor nit, while I was checking where testDestroyVport gets called from
- testStoragePoolDestroy, there's a spot that could be fixed the same way.
Would you mind squashing this bit in as well?
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6422ba8fb..a397364c7 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
virObjectEventPtr event = NULL;
if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name)))
- goto cleanup;
+ return -1;
if (!virStoragePoolObjIsActive(privpool)) {
virReportError(VIR_ERR_OPERATION_INVALID,
[...]
> @@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
> virNodeDeviceDefPtr def;
> virNodeDevCapsDefPtr caps;
> int ncaps = 0;
> - int ret = -1;
>
> if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
> - goto cleanup;
> + return -1;
> def = virNodeDeviceObjGetDef(obj);
>
> for (caps = 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++;
How about placing the increment to the iteration_expression segment of the
loop, aka at the end where the usual increment happens before condition
evaluation. Wondering whether there's a general technical term for the syntax
part of the loop between the parentheses.
> }
> - ret = ncaps;
>
> - cleanup:
> - if (obj)
> - virNodeDeviceObjUnlock(obj);
> - if (ret == -1) {
> - --ncaps;
> - while (--ncaps >= 0)
> - VIR_FREE(names[ncaps]);
Hmm, this was indeed an interesting bit of code.
> - }
> - return ret;
> + virNodeDeviceObjUnlock(obj);
> + return ncaps;
> +
> + error:
> + while (--ncaps >= 0)
> + VIR_FREE(names[ncaps]);
> + virNodeDeviceObjUnlock(obj);
> + return -1;
> }
>
ACK with adjustments.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/29/2017 08:57 AM, Erik Skultety wrote:
> On Sat, Jun 03, 2017 at 09:11:52AM -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>
>
> [..]
>
>> event = virNodeDeviceEventLifecycleNew("scsi_host12",
>> @@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn,
>> virNodeDeviceObjFree(obj);
>> obj = NULL;
>
> You can drop ^this then since you don't need to clear it anymore.
>
True, I'll drop it especially since the reason it was there to avoid the
virNodeDeviceObjUnlock call is no longer valid.
> Just a minor nit, while I was checking where testDestroyVport gets called from
> - testStoragePoolDestroy, there's a spot that could be fixed the same way.
> Would you mind squashing this bit in as well?
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 6422ba8fb..a397364c7 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
> virObjectEventPtr event = NULL;
>
> if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name)))
> - goto cleanup;
> + return -1;
>
I don't mind changing this as well - although I probably have that in
another patch somewhere dealing with storage tests. Trying to keep
nodedev with nodedev and storage with storage.
> if (!virStoragePoolObjIsActive(privpool)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
>
>
>
> [...]
>
>> @@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
>> virNodeDeviceDefPtr def;
>> virNodeDevCapsDefPtr caps;
>> int ncaps = 0;
>> - int ret = -1;
>>
>> if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
>> - goto cleanup;
>> + return -1;
>> def = virNodeDeviceObjGetDef(obj);
>>
>> for (caps = 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++;
>
> How about placing the increment to the iteration_expression segment of the
> loop, aka at the end where the usual increment happens before condition
> evaluation. Wondering whether there's a general technical term for the syntax
> part of the loop between the parentheses.
>
Do you mean :
(caps = def->caps; caps && ncaps < maxnames; caps = caps->next, ncaps++)
??
If so, yuck. Since we're iterating on caps, I think what should be
incremented is caps and not ncaps as well. Putting it after the
VIR_STRDUP() just makes it clearer that we successfully added something.
I'd prefer to keep as is.
Tks -
John
>> }
>> - ret = ncaps;
>>
>> - cleanup:
>> - if (obj)
>> - virNodeDeviceObjUnlock(obj);
>> - if (ret == -1) {
>> - --ncaps;
>> - while (--ncaps >= 0)
>> - VIR_FREE(names[ncaps]);
>
> Hmm, this was indeed an interesting bit of code.
>
>> - }
>> - return ret;
>> + virNodeDeviceObjUnlock(obj);
>> + return ncaps;
>> +
>> + error:
>> + while (--ncaps >= 0)
>> + VIR_FREE(names[ncaps]);
>> + virNodeDeviceObjUnlock(obj);
>> + return -1;
>> }
>>
>
> ACK with adjustments.
>
> Erik
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> > Just a minor nit, while I was checking where testDestroyVport gets called from
> > - testStoragePoolDestroy, there's a spot that could be fixed the same way.
> > Would you mind squashing this bit in as well?
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 6422ba8fb..a397364c7 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
> > virObjectEventPtr event = NULL;
> >
> > if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name)))
> > - goto cleanup;
> > + return -1;
> >
>
> I don't mind changing this as well - although I probably have that in
> another patch somewhere dealing with storage tests. Trying to keep
> nodedev with nodedev and storage with storage.
I figured, but this is where these two world clash, you know, calling nodedev
stuff from storage... I don't care if you fix in this one or in some other
storage related patch, just so we don't forget about that.
>
>
> > if (!virStoragePoolObjIsActive(privpool)) {
> > virReportError(VIR_ERR_OPERATION_INVALID,
> >
> >
> >
> > [...]
> >
> >> @@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
> >> virNodeDeviceDefPtr def;
> >> virNodeDevCapsDefPtr caps;
> >> int ncaps = 0;
> >> - int ret = -1;
> >>
> >> if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
> >> - goto cleanup;
> >> + return -1;
> >> def = virNodeDeviceObjGetDef(obj);
> >>
> >> for (caps = 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++;
> >
> > How about placing the increment to the iteration_expression segment of the
> > loop, aka at the end where the usual increment happens before condition
> > evaluation. Wondering whether there's a general technical term for the syntax
> > part of the loop between the parentheses.
> >
>
> Do you mean :
>
> (caps = def->caps; caps && ncaps < maxnames; caps = caps->next, ncaps++)
>
> ??
>
> If so, yuck. Since we're iterating on caps, I think what should be
> incremented is caps and not ncaps as well. Putting it after the
Well, if we only cared about the @caps in the evaluation condition, maybe, but
we also check @ncaps and I think it's pretty clear that ncaps are only
incremented when VIR_STRDUP passed, otherwise we would have dropped from the
loop in the first place. I don't intend to argue about this, as it's not a show
stopper, so I don't really care that much about that, I'm fine either way.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.