[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs




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
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]