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

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(a)redhat.com&gt; > --- [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.

Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;