[libvirt] [PATCH 02/19] test: Use consistent variable names for storage test driver APIs

[John Ferlan]

On 07/11/2017 04:27 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:09AM -0400, John Ferlan wrote:
>> A virStoragePoolObjPtr will be an 'obj'.
>>
>> A virStoragePoolPtr will be a 'pool'.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/test/test_driver.c | 443 ++++++++++++++++++++++++-------------------------
>>  1 file changed, 219 insertions(+), 224 deletions(-)
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 548f318..c0e46af 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
> 
> [...]
> 
>> @@ -4057,18 +4056,18 @@ testStoragePoolLookupByUUID(virConnectPtr conn,
>>                              const unsigned char *uuid)
>>  {
>>      testDriverPtr privconn = conn->privateData;
>> -    virStoragePoolObjPtr pool;
>> +    virStoragePoolObjPtr obj;
>>      virStoragePoolPtr ret = NULL;
> 
> There you didn't changed the "ret" to "pool".
> 
>> -    if (!(pool = testStoragePoolObjFindByUUID(privconn, uuid)))
>> +    if (!(obj = testStoragePoolObjFindByUUID(privconn, uuid)))
>>          goto cleanup;
>>  
>> -    ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
>> +    ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid,
>>                              NULL, NULL);
>>  
>>   cleanup:
>> -    if (pool)
>> -        virStoragePoolObjUnlock(pool);
>> +    if (obj)
>> +        virStoragePoolObjUnlock(obj);
>>      return ret;
>>  }
>>  
>> @@ -4078,18 +4077,18 @@ testStoragePoolLookupByName(virConnectPtr conn,
>>                              const char *name)
>>  {
>>      testDriverPtr privconn = conn->privateData;
>> -    virStoragePoolObjPtr pool;
>> +    virStoragePoolObjPtr obj;
>>      virStoragePoolPtr ret = NULL;
> 
> Same here.
> 
>> -    if (!(pool = testStoragePoolObjFindByName(privconn, name)))
>> +    if (!(obj = testStoragePoolObjFindByName(privconn, name)))
>>          goto cleanup;
>>  
>> -    ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
>> +    ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid,
>>                              NULL, NULL);
>>  
>>   cleanup:
>> -    if (pool)
>> -        virStoragePoolObjUnlock(pool);
>> +    if (obj)
>> +        virStoragePoolObjUnlock(obj);
>>      return ret;
>>  }
>>  
> 
> [...]
> 
>> @@ -4345,7 +4344,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
>>  {
>>      testDriverPtr privconn = conn->privateData;
>>      virStoragePoolDefPtr def;
>> -    virStoragePoolObjPtr pool = NULL;
>> +    virStoragePoolObjPtr obj = NULL;
>>      virStoragePoolPtr ret = NULL;
> 
> And here.
> 
>>      virObjectEventPtr event = NULL;
>>  
> 
> [...]
> 
>> @@ -4419,7 +4418,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
>>  {
>>      testDriverPtr privconn = conn->privateData;
>>      virStoragePoolDefPtr def;
>> -    virStoragePoolObjPtr pool = NULL;
>> +    virStoragePoolObjPtr obj = NULL;
>>      virStoragePoolPtr ret = NULL;
> 
> And here
> 

I can adjust those... I was more focused on the "existing" @pool and
@obj variables and didn't consider the existing @ret variables.

>>      virObjectEventPtr event = NULL;
>>
> 
> [...]
> 
> I don't like these type of patches.  The value to noise ration is poor.
> I'm hesitating to give this patch an ACK even though I probably done
> that in the past.
> 
> Pavel
> 

So while I understand the concern, my argument becomes is it technically
incorrect to change the names?  Secondary to that if you're considering
multiple driver/vir*obj changes at one time, it is *far easier* to
consider an "@obj" to be that thing in the list vir*obj.c list rather
than what is either passed or gets returned from/to the consumer - a
@pool. And yes, technically a @pool is an object - I know.

The second thing that gets fixed by these changes is inconsistent usage.
For instance, prior to these changes look at testParseStorage and
testOpenVolumesForPool. The former uses @obj for a virStoragePoolObjPtr
while the latter uses @pool for a virStoragePoolObjPtr. It becomes
difficult to "follow" code with inconsistencies. So if someone takes the
effort to make things consistent - I think that's better in the long
run. Makes the code more maintainable for a reader, but yes a hassle for
someone back porting some subsequent change because of the merge conflict.

The make code more consistent wins the argument my left and right brain
have over this.

If there's not an ACK, then so be it - I can remove it, but it probably
affects subsequent patches too.

Thanks for going back to earlier series - I had partially given up on
storage and network instead focusing more on secret, nodedev, interface,
and nwfilter. I figured once I got those accepted, I could go back to
storage and network later. This just reduces that workload ;-)

John