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@xxxxxxxxxx>
>> ---
>
>> @@ -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
