1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 0/4] More virsecretobj changes to prepare for common object
  4. View patch

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

John Ferlan posted 4 patches 7 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v2 1/4] secret: Clean up virSecretObjListAdd processing
Posted by John Ferlan 7 years, 10 months ago 
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.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virsecretobj.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index e3bcbe5..bedcdbd 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)
@@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
             virSecretDefFree(objdef);
         obj->def = newdef;
     } else {
+
         /* No existing secret with same UUID,
-         * try look for matching usage instead */
+         * try to look for matching usage instead */
         if ((obj = virSecretObjListFindByUsageLocked(secrets,
                                                      newdef->usage_type,
                                                      newdef->usage_id))) {
@@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
                            _("a secret with UUID %s already defined for "
                              "use with %s"),
                            uuidstr, newdef->usage_id);
-            goto cleanup;
+            goto error;
         }
 
         /* Generate the possible configFile and base64File strings
@@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
             goto cleanup;
 
         if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
-            goto cleanup;
+            goto error;
 
         obj->def = newdef;
         VIR_STEAL_PTR(obj->configFile, configFile);
@@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
         virObjectRef(obj);
     }
 
-    ret = obj;
-    obj = NULL;
-
  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
Re: [libvirt] [PATCH v2 1/4] secret: Clean up virSecretObjListAdd processing
Posted by Pavel Hrdina 7 years, 9 months ago 
On Fri, Jul 14, 2017 at 10:04:39AM -0400, 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.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virsecretobj.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index e3bcbe5..bedcdbd 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)
> @@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>              virSecretDefFree(objdef);
>          obj->def = newdef;
>      } else {
> +
>          /* No existing secret with same UUID,
> -         * try look for matching usage instead */
> +         * try to look for matching usage instead */
>          if ((obj = virSecretObjListFindByUsageLocked(secrets,
>                                                       newdef->usage_type,
>                                                       newdef->usage_id))) {
> @@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>                             _("a secret with UUID %s already defined for "
>                               "use with %s"),
>                             uuidstr, newdef->usage_id);
> -            goto cleanup;
> +            goto error;
>          }
>  
>          /* Generate the possible configFile and base64File strings
> @@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>              goto cleanup;
>  
>          if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> -            goto cleanup;
> +            goto error;
>  
>          obj->def = newdef;
>          VIR_STEAL_PTR(obj->configFile, configFile);
> @@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>          virObjectRef(obj);
>      }
>  
> -    ret = obj;
> -    obj = NULL;
> -
>   cleanup:
> -    virSecretObjEndAPI(&obj);
>      VIR_FREE(configFile);
>      VIR_FREE(base64File);
>      virObjectUnlock(secrets);
> -    return ret;
> +    return obj;
> +
> + error:
> +    virSecretObjEndAPI(&obj);
> +    goto cleanup;

I don't see what's wrong with the current code, it's commonly used
pattern to steal the pointer.  The extra error label would make sense
if the error path would be more complex, but in this case it doesn't
add any value.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] secret: Clean up virSecretObjListAdd processing
Posted by John Ferlan 7 years, 9 months ago 

On 07/25/2017 07:21 AM, Pavel Hrdina wrote:
> On Fri, Jul 14, 2017 at 10:04:39AM -0400, 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.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virsecretobj.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index e3bcbe5..bedcdbd 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)
>> @@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>              virSecretDefFree(objdef);
>>          obj->def = newdef;
>>      } else {
>> +
>>          /* No existing secret with same UUID,
>> -         * try look for matching usage instead */
>> +         * try to look for matching usage instead */
>>          if ((obj = virSecretObjListFindByUsageLocked(secrets,
>>                                                       newdef->usage_type,
>>                                                       newdef->usage_id))) {
>> @@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>                             _("a secret with UUID %s already defined for "
>>                               "use with %s"),
>>                             uuidstr, newdef->usage_id);
>> -            goto cleanup;
>> +            goto error;
>>          }
>>  
>>          /* Generate the possible configFile and base64File strings
>> @@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>              goto cleanup;
>>  
>>          if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
>> -            goto cleanup;
>> +            goto error;
>>  
>>          obj->def = newdef;
>>          VIR_STEAL_PTR(obj->configFile, configFile);
>> @@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>          virObjectRef(obj);
>>      }
>>  
>> -    ret = obj;
>> -    obj = NULL;
>> -
>>   cleanup:
>> -    virSecretObjEndAPI(&obj);
>>      VIR_FREE(configFile);
>>      VIR_FREE(base64File);
>>      virObjectUnlock(secrets);
>> -    return ret;
>> +    return obj;
>> +
>> + error:
>> +    virSecretObjEndAPI(&obj);
>> +    goto cleanup;
> 
> I don't see what's wrong with the current code, it's commonly used
> pattern to steal the pointer.  The extra error label would make sense
> if the error path would be more complex, but in this case it doesn't
> add any value.
> 
> Pavel
> 

Fine - I'll drop this one then.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.