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

On 07/11/2017 11:52 AM, Michal Privoznik wrote: > 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 > I'll return the if - else - logic... and update the commit message. Would you like to see a version of that?

It does affect the next couple of patches. For patch 5 it's just a simple indention adjustment. Since there's questions in 6 I'll address those there.