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