[libvirt] [PATCH v2 12/32] storage: Use VIR_AUTOFREE for storage backends

[John Ferlan]

On 2/11/19 5:39 AM, Erik Skultety wrote:
> On Fri, Feb 08, 2019 at 01:37:06PM -0500, John Ferlan wrote:
>> Let's make use of the auto __cleanup capabilities. This also allows
>> for the cleanup of some goto paths.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
> 
>> @@ -459,15 +454,15 @@ virStorageBackendLogicalFindPoolSourcesFunc(char **const groups,
>>                                              void *data)
>>  {
>>      virStoragePoolSourceListPtr sourceList = data;
>> -    char *pvname = NULL;
>> -    char *vgname = NULL;
>>      size_t i;
>>      virStoragePoolSourceDevicePtr dev;
>>      virStoragePoolSource *thisSource;
>> +    VIR_AUTOFREE(char *) pvname = NULL;
>> +    VIR_AUTOFREE(char *) vgname = NULL;
>>
>>      if (VIR_STRDUP(pvname, groups[0]) < 0 ||
>>          VIR_STRDUP(vgname, groups[1]) < 0)
>> -        goto error;
>> +        return -1;
>>
>>      thisSource = NULL;
>>      for (i = 0; i < sourceList->nsources; i++) {
>> @@ -479,30 +474,22 @@ virStorageBackendLogicalFindPoolSourcesFunc(char **const groups,
>>
>>      if (thisSource == NULL) {
>>          if (!(thisSource = virStoragePoolSourceListNewSource(sourceList)))
>> -            goto error;
>> +            return -1;
>>
>> -        thisSource->name = vgname;
>> +        VIR_STEAL_PTR(thisSource->name, vgname);
>>      }
>> -    else
>> -        VIR_FREE(vgname);
>>
>>      if (VIR_REALLOC_N(thisSource->devices, thisSource->ndevice + 1) != 0)
>> -        goto error;
>> +        return -1;
>>
>>      dev = &thisSource->devices[thisSource->ndevice];
>>      thisSource->ndevice++;
>>      thisSource->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
>>
>>      memset(dev, 0, sizeof(*dev));
>> -    dev->path = pvname;
>> +    VIR_STEAL_PTR(dev->path, pvname);
> 
> I still don't see why ^this needs to stay and can't be separated into a
> preceding patch.
> 

Because in my view/mind - previously there was no problem for pvname in
the success path; however, with this change and without a VIR_STEAL_PTR
there would be a problem.

Moving the VIR_STEAL_PTR to a/the previous patch for this line is a noop
since we never fall through to the err: label.

I suppose if you insist I can move it as it really doesn't matter that
much to me.

>>
>>      return 0;
>> -
>> - error:
>> -    VIR_FREE(pvname);
>> -    VIR_FREE(vgname);
>> -
>> -    return -1;
>>  }
> 
> ...
> 
>>
>> diff --git a/src/storage/storage_file_gluster.c b/src/storage/storage_file_gluster.c
>> index f8bbde8cfe..7c2189d297 100644
>> --- a/src/storage/storage_file_gluster.c
>> +++ b/src/storage/storage_file_gluster.c
>> @@ -258,10 +258,10 @@ virStorageFileBackendGlusterReadlinkCallback(const char *path,
>>                                               void *data)
>>  {
>>      virStorageFileBackendGlusterPrivPtr priv = data;
>> -    char *buf = NULL;
>>      size_t bufsiz = 0;
>>      ssize_t ret;
>>      struct stat st;
>> +    VIR_AUTOFREE(char *) buf = NULL;
>>
>>      *linkpath = NULL;
>>
>> @@ -277,13 +277,13 @@ virStorageFileBackendGlusterReadlinkCallback(const char *path,
>>
>>   realloc:
>>      if (VIR_EXPAND_N(buf, bufsiz, 256) < 0)
>> -        goto error;
>> +        return -1;
>>
>>      if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) {
>>          virReportSystemError(errno,
>>                               _("failed to read link of gluster file '%s'"),
>>                               path);
>> -        goto error;
>> +        return -1;
>>      }
>>
>>      if (ret == bufsiz)
>> @@ -291,13 +291,9 @@ virStorageFileBackendGlusterReadlinkCallback(const char *path,
>>
>>      buf[ret] = '\0';
>>
>> -    *linkpath = buf;
>> +    VIR_STEAL_PTR(*linkpath, buf);
> 
> ^This VIR_STEAL_PTR can also be separated, it doesn't depend on the
> VIR_AUTOFREE stuff, it's a simple 1 change hunk.
> 

Same reasoning here.

John