Re: [libvirt] [PATCH v2 1/4] secret: Clean up virSecretObjListAdd processing

On Fri, Jul 14, 2017 at 10:04:39AM -0400, John Ferlan wrote: > Make use of an error: label to handle the failure and need to call > virSecretObjEndAPI for the object to set it to NULL for return. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/virsecretobj.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c > index e3bcbe5..bedcdbd 100644 > --- a/src/conf/virsecretobj.c > +++ b/src/conf/virsecretobj.c > @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > { > virSecretObjPtr obj; > virSecretDefPtr objdef; > - virSecretObjPtr ret = NULL; > char uuidstr[VIR_UUID_STRING_BUFLEN]; > char *configFile = NULL, *base64File = NULL; > > @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > _("a secret with UUID %s is already defined for " > "use with %s"), > uuidstr, objdef->usage_id); > - goto cleanup; > + goto error; > } > > if (objdef->isprivate && !newdef->isprivate) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("cannot change private flag on existing secret")); > - goto cleanup; > + goto error; > } > > if (oldDef) > @@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > virSecretDefFree(objdef); > obj->def = newdef; > } else { > + > /* No existing secret with same UUID, > - * try look for matching usage instead */ > + * try to look for matching usage instead */ > if ((obj = virSecretObjListFindByUsageLocked(secrets, > newdef->usage_type, > newdef->usage_id))) { > @@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > _("a secret with UUID %s already defined for " > "use with %s"), > uuidstr, newdef->usage_id); > - goto cleanup; > + goto error; > } > > /* Generate the possible configFile and base64File strings > @@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > goto cleanup; > > if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) > - goto cleanup; > + goto error; > > obj->def = newdef; > VIR_STEAL_PTR(obj->configFile, configFile); > @@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > virObjectRef(obj); > } > > - ret = obj; > - obj = NULL; > - > cleanup: > - virSecretObjEndAPI(&obj); > VIR_FREE(configFile); > VIR_FREE(base64File); > virObjectUnlock(secrets); > - return ret; > + return obj; > + > + error: > + virSecretObjEndAPI(&obj); > + goto cleanup; I don't see what's wrong with the current code, it's commonly used pattern to steal the pointer. The extra error label would make sense if the error path would be more complex, but in this case it doesn't add any value. Pavel