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@redhat.com> --- 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