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

[Pavel Hrdina]

On Thu, Jul 13, 2017 at 03:40:26PM -0400, John Ferlan wrote:
> 
> 
> 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.

It's my personal feeling that there is no much value in this patch,
however the code will be at least consistent.

Reviewed-by: Pavel Hrdina <phrdina at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170714/9afd5eff/attachment-0001.sig>