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