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

[Michal Privoznik]

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 at 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