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@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;
- ret = obj;
- obj = NULL;
+ obj->def = newdef;
+ VIR_STEAL_PTR(obj->configFile, configFile);
+ VIR_STEAL_PTR(obj->base64File, base64File);
+ virObjectRef(obj);
cleanup:
- virSecretObjEndAPI(&obj);
VIR_FREE(configFile);
VIR_FREE(base64File);
virObjectUnlock(secrets);
- return ret;
+ return obj;
+
+ error:
+ virSecretObjEndAPI(&obj);
+ goto cleanup;
}
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.