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

On 06/03/2017 03:27 PM, 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. > > Also rather than an if/else processing - have the initial "if" which > is just replacing the @newdef into obj->def goto cleanup, thus allowing > the remaining code to be extracted from the else and appear to more inline. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++------------------------- > 1 file changed, 37 insertions(+), 37 deletions(-) > > diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c > index e3bcbe5..1bafd0b 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) > @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets, > else > virSecretDefFree(objdef); > obj->def = newdef; > - } else { > - /* No existing secret with same UUID, > - * try look for matching usage instead */ > - if ((obj = virSecretObjListFindByUsageLocked(secrets, > - newdef->usage_type, > - newdef->usage_id))) { > - virObjectLock(obj); > - objdef = obj->def; > - virUUIDFormat(objdef->uuid, uuidstr); > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("a secret with UUID %s already defined for " > - "use with %s"), > - uuidstr, newdef->usage_id); > - goto cleanup; > - } > + goto cleanup; > + } > > - /* Generate the possible configFile and base64File strings > - * using the configDir, uuidstr, and appropriate suffix > - */ > - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || > - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) > - goto cleanup; > + /* No existing secret with same UUID, > + * try to look for matching usage instead */ > + if ((obj = virSecretObjListFindByUsageLocked(secrets, > + newdef->usage_type, > + newdef->usage_id))) { > + virObjectLock(obj); > + objdef = obj->def; > + virUUIDFormat(objdef->uuid, uuidstr); > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("a secret with UUID %s already defined for " > + "use with %s"), > + uuidstr, newdef->usage_id); > + goto error; > + } > > - if (!(obj = virSecretObjNew())) > - goto cleanup; > + /* Generate the possible configFile and base64File strings > + * using the configDir, uuidstr, and appropriate suffix > + */ > + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || > + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) > + goto cleanup; > > - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) > - goto cleanup; > + if (!(obj = virSecretObjNew())) > + goto cleanup; > > - obj->def = newdef; > - VIR_STEAL_PTR(obj->configFile, configFile); > - VIR_STEAL_PTR(obj->base64File, base64File); > - virObjectRef(obj); > - } > + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) > + goto error; Frankly, I find this very confusing. While reading the commit message, I understand why you sometimes jump to cleanup and other times to error label. But if I were to just look at the code alone, it's completely random to me. I think I'd much rather see the pattern that's here. However, if you really dislike if-else branching (dislike also as in prepare for upcoming patches), I suggest changing just that. Michal