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

[John Ferlan]

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

John