[libvirt] [PATCH 08/19] storage: Use consistent variable names for driver

[John Ferlan]

On 07/14/2017 11:07 AM, Pavel Hrdina wrote:
> On Tue, May 09, 2017 at 11:30:15AM -0400, John Ferlan wrote:
>> A virStoragePoolObjPtr will be an 'obj'.
>>
>> A virStoragePoolPtr will be a 'pool'.
>>
>> A virStorageVolPtr will be a 'vol'.
>>
>> A virStorageVolDefPtr will be a 'voldef'.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/storage/storage_driver.c | 1158 +++++++++++++++++++++---------------------
>>  src/storage/storage_driver.h |    4 +-
>>  2 files changed, 582 insertions(+), 580 deletions(-)
>>

I have had some more time to think about the other comment regarding
whether @obj should be @poolObj or @poolobj...

If some day in the future (as noted in the patch 7 response) the @pools
changes to @poolobjs it'll be eye test in order to spot the difference
between @poolobj and @poolobjs, so I'd like to keep it as @obj. Long
time ago I had the sheer joy of trying to search code for 'l' and '1' as
well as 'O' and '0'

>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 2103ed1..6122396 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c

[...] lots to cut to first comment I saw [...]

>>  
>>  static virStorageVolPtr
>> -storageVolCreateXML(virStoragePoolPtr obj,
>> +storageVolCreateXML(virStoragePoolPtr pool,
>>                      const char *xmldesc,
>>                      unsigned int flags)
>>  {
>> -    virStoragePoolObjPtr pool;
>> +    virStoragePoolObjPtr obj;
>>      virStorageBackendPtr backend;
>>      virStorageVolDefPtr voldef = NULL;
>> -    virStorageVolPtr ret = NULL, volobj = NULL;
>> +    virStorageVolPtr vol = NULL, volobj = NULL;
> 
> The @volobj should be also renamed, I would rename it like this:
> 
>     @ret -> @vol
>     @volobj -> @newvol
> 
> and ...
> 
>>  
>>      virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>>  
>> -    if (!(pool = virStoragePoolObjFromStoragePool(obj)))
>> +    if (!(obj = virStoragePoolObjFromStoragePool(pool)))
>>          return NULL;
>>  
>> -    if (!virStoragePoolObjIsActive(pool)) {
>> +    if (!virStoragePoolObjIsActive(obj)) {
>>          virReportError(VIR_ERR_OPERATION_INVALID,
>> -                       _("storage pool '%s' is not active"), pool->def->name);
>> +                       _("storage pool '%s' is not active"), obj->def->name);
>>          goto cleanup;
>>      }
>>  
>> -    if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
>> +    if ((backend = virStorageBackendForType(obj->def->type)) == NULL)
>>          goto cleanup;
>>  
>> -    voldef = virStorageVolDefParseString(pool->def, xmldesc,
>> +    voldef = virStorageVolDefParseString(obj->def, xmldesc,
>>                                           VIR_VOL_XML_PARSE_OPT_CAPACITY);
>>      if (voldef == NULL)
>>          goto cleanup;
>> @@ -1799,10 +1799,10 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>          goto cleanup;
>>      }
>>  
>> -    if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0)
>> +    if (virStorageVolCreateXMLEnsureACL(pool->conn, obj->def, voldef) < 0)
>>          goto cleanup;
>>  
>> -    if (virStorageVolDefFindByName(pool, voldef->name)) {
>> +    if (virStorageVolDefFindByName(obj, voldef->name)) {
>>          virReportError(VIR_ERR_STORAGE_VOL_EXIST,
>>                         _("'%s'"), voldef->name);
>>          goto cleanup;
>> @@ -1815,21 +1815,21 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>          goto cleanup;
>>      }
>>  
>> -    if (VIR_REALLOC_N(pool->volumes.objs,
>> -                      pool->volumes.count+1) < 0)
>> +    if (VIR_REALLOC_N(obj->volumes.objs,
>> +                      obj->volumes.count + 1) < 0)
>>          goto cleanup;
>>  
>>      /* Wipe any key the user may have suggested, as volume creation
>>       * will generate the canonical key.  */
>>      VIR_FREE(voldef->key);
>> -    if (backend->createVol(obj->conn, pool, voldef) < 0)
>> +    if (backend->createVol(pool->conn, obj, voldef) < 0)
>>          goto cleanup;
>>  
>> -    pool->volumes.objs[pool->volumes.count++] = voldef;
>> -    volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
>> +    obj->volumes.objs[obj->volumes.count++] = voldef;
>> +    volobj = virGetStorageVol(pool->conn, obj->def->name, voldef->name,
>>                                voldef->key, NULL, NULL);
>>      if (!volobj) {
>> -        pool->volumes.count--;
>> +        obj->volumes.count--;
>>          goto cleanup;
>>      }
>>  
>> @@ -1850,24 +1850,24 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>          memcpy(buildvoldef, voldef, sizeof(*voldef));
>>  
>>          /* Drop the pool lock during volume allocation */
>> -        pool->asyncjobs++;
>> +        obj->asyncjobs++;
>>          voldef->building = true;
>> -        virStoragePoolObjUnlock(pool);
>> +        virStoragePoolObjUnlock(obj);
>>  
>> -        buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags);
>> +        buildret = backend->buildVol(pool->conn, obj, buildvoldef, flags);
>>  
>>          VIR_FREE(buildvoldef);
>>  
>>          storageDriverLock();
>> -        virStoragePoolObjLock(pool);
>> +        virStoragePoolObjLock(obj);
>>          storageDriverUnlock();
>>  
>>          voldef->building = false;
>> -        pool->asyncjobs--;
>> +        obj->asyncjobs--;
>>  
>>          if (buildret < 0) {
>>              /* buildVol handles deleting volume on failure */
>> -            storageVolRemoveFromPool(pool, voldef);
>> +            storageVolRemoveFromPool(obj, voldef);
>>              voldef = NULL;
>>              goto cleanup;
>>          }
>> @@ -1875,8 +1875,8 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>      }
>>  
>>      if (backend->refreshVol &&
>> -        backend->refreshVol(obj->conn, pool, voldef) < 0) {
>> -        storageVolDeleteInternal(volobj, backend, pool, voldef,
>> +        backend->refreshVol(pool->conn, obj, voldef) < 0) {
>> +        storageVolDeleteInternal(volobj, backend, obj, voldef,
>>                                   0, false);
>>          voldef = NULL;
>>          goto cleanup;
>> @@ -1885,35 +1885,39 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>      /* Update pool metadata ignoring the disk backend since
>>       * it updates the pool values.
>>       */
>> -    if (pool->def->type != VIR_STORAGE_POOL_DISK) {
>> -        pool->def->allocation += voldef->target.allocation;
>> -        pool->def->available -= voldef->target.allocation;
>> +    if (obj->def->type != VIR_STORAGE_POOL_DISK) {
>> +        obj->def->allocation += voldef->target.allocation;
>> +        obj->def->available -= voldef->target.allocation;
>>      }
>>  
>>      VIR_INFO("Creating volume '%s' in storage pool '%s'",
>> -             volobj->name, pool->def->name);
>> -    ret = volobj;
>> +             volobj->name, obj->def->name);
>> +    vol = volobj;
>>      volobj = NULL;
>>      voldef = NULL;
>>  
>>   cleanup:
>>      virObjectUnref(volobj);
>>      virStorageVolDefFree(voldef);
>> -    if (pool)
>> -        virStoragePoolObjUnlock(pool);
>> -    return ret;
>> +    if (obj)
>> +        virStoragePoolObjUnlock(obj);
>> +    return vol;
>>  }
>>  
>>  static virStorageVolPtr
>> -storageVolCreateXMLFrom(virStoragePoolPtr obj,
>> +storageVolCreateXMLFrom(virStoragePoolPtr pool,
>>                          const char *xmldesc,
>> -                        virStorageVolPtr vobj,
>> +                        virStorageVolPtr volsrc,
>>                          unsigned int flags)
>>  {
>> -    virStoragePoolObjPtr pool, origpool = NULL;
>> +    virStoragePoolObjPtr obj;
>> +    virStoragePoolObjPtr objsrc = NULL;
>>      virStorageBackendPtr backend;
>> -    virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
>> -    virStorageVolPtr ret = NULL, volobj = NULL;
>> +    virStorageVolDefPtr voldefsrc = NULL;
>> +    virStorageVolDefPtr voldef = NULL;
>> +    virStorageVolDefPtr shadowvol = NULL;
>> +    virStorageVolPtr volobj = NULL;
>> +    virStorageVolPtr vol = NULL;
> 
> ... same here.
> 
> Pavel
> 

Works for me - consider it done.

John