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