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

[Michal Privoznik]

On 07/13/2017 06:36 PM, John Ferlan wrote:
> 
> 
> 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 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
>>
> 
> I'll return the if - else - logic...  and update the commit message.
> 
> Would you like to see a version of that?

Yes please.

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

That's okay, you can just send another version (and state that some
patches were ACKed already).

Michal