[libvirt] [PATCH 09/15] storage: Use VIR_AUTOFREE for storage util

[John Ferlan]

On 2/7/19 9:32 AM, Erik Skultety wrote:
> On Wed, Feb 06, 2019 at 08:41:41AM -0500, John Ferlan wrote:
>> Let's make use of the auto __cleanup capabilities cleaning up any
>> now unnecessary goto paths.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
> 
> [snip]
> 
>> @@ -3804,7 +3713,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>  {
>>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>>      VIR_AUTOPTR(virStorageVolDef) vol = NULL;
>> -    char *devpath = NULL;
>> +    VIR_AUTOFREE(char *) devpath = NULL;
>>      int retval = -1;
>>
>>      /* Check if the pool is using a stable target path. The call to
>> @@ -3820,11 +3729,11 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>          virReportError(VIR_ERR_INVALID_ARG,
>>                         _("unable to use target path '%s' for dev '%s'"),
>>                         NULLSTR(def->target.path), dev);
>> -        goto cleanup;
>> +        return -1;
>>      }
>>
>>      if (VIR_ALLOC(vol) < 0)
>> -        goto cleanup;
>> +        return -1;
>>
>>      vol->type = VIR_STORAGE_VOL_BLOCK;
>>
>> @@ -3834,10 +3743,10 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>       * just leave 'host' out
>>       */
>>      if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0)
>> -        goto cleanup;
>> +        return -1;
>>
>>      if (virAsprintf(&devpath, "/dev/%s", dev) < 0)
>> -        goto cleanup;
>> +        return -1;
>>
>>      VIR_DEBUG("Trying to create volume for '%s'", devpath);
>>
>> @@ -3850,7 +3759,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>      if ((vol->target.path = virStorageBackendStablePath(pool,
>>                                                          devpath,
>>                                                          true)) == NULL)
>> -        goto cleanup;
>> +        return -1;
>>
>>      if (STREQ(devpath, vol->target.path) &&
>>          !(STREQ(def->target.path, "/dev") ||
>> @@ -3859,34 +3768,29 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>          VIR_DEBUG("No stable path found for '%s' in '%s'",
>>                    devpath, def->target.path);
>>
>> -        retval = -2;
>> -        goto cleanup;
>> +        return -2;
>>      }
>>
>>      /* Allow a volume read failure to ignore or skip this block file */
>>      if ((retval = virStorageBackendUpdateVolInfo(vol, true,
>>                                                   VIR_STORAGE_VOL_OPEN_DEFAULT,
>>                                                   VIR_STORAGE_VOL_READ_NOERROR)) < 0)
>> -        goto cleanup;
>> +        return retval;
>>
>>      vol->key = virStorageBackendSCSISerial(vol->target.path,
>>                                             (def->source.adapter.type ==
>>                                              VIR_STORAGE_ADAPTER_TYPE_FC_HOST));
>>      if (!vol->key)
>> -        goto cleanup;
> 
> We're changing the logic ^here - previously if virStorageBackendUpdateVolInfo
> succeeded but virStorageBackendSCSISerial returned NULL, we'd still return
> retval which would have been equal to 0. To me, your change feels right, but I
> want to make sure no caller was relying on 0 even though
> virStorageBackendSCSISerial might have failed.

That's my take too - introduced by a523770c3.. Looking at the logic for
the purpose of allowing the *VolInfo to return -2... F*d up the non
error path though if virStorageBackendSCSISerial failed.

I can separate this for clarity...

John

> 
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
>