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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
