By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
---
 src/util/viridentity.c | 54 ++++++++++++++++++++------------------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 2f4307b..2060dd7 100644
--- a/src/util/viridentity.c
+++ b/src/util/viridentity.c
@@ -133,8 +133,8 @@ int virIdentitySetCurrent(virIdentityPtr ident)
  */
 virIdentityPtr virIdentityGetSystem(void)
 {
-    char *username = NULL;
-    char *groupname = NULL;
+    VIR_AUTOFREE(char *) username = NULL;
+    VIR_AUTOFREE(char *) groupname = NULL;
     unsigned long long startTime;
     virIdentityPtr ret = NULL;
 #if WITH_SELINUX
@@ -154,14 +154,14 @@ virIdentityPtr virIdentityGetSystem(void)
         goto error;
 
     if (!(username = virGetUserName(geteuid())))
-        goto cleanup;
+        return ret;
     if (virIdentitySetUNIXUserName(ret, username) < 0)
         goto error;
     if (virIdentitySetUNIXUserID(ret, getuid()) < 0)
         goto error;
 
     if (!(groupname = virGetGroupName(getegid())))
-        goto cleanup;
+        return ret;
     if (virIdentitySetUNIXGroupName(ret, groupname) < 0)
         goto error;
     if (virIdentitySetUNIXGroupID(ret, getgid()) < 0)
@@ -172,7 +172,7 @@ virIdentityPtr virIdentityGetSystem(void)
         if (getcon(&con) < 0) {
             virReportSystemError(errno, "%s",
                                  _("Unable to lookup SELinux process context"));
-            goto cleanup;
+            return ret;
         }
         if (virIdentitySetSELinuxContext(ret, con) < 0) {
             freecon(con);
@@ -182,15 +182,9 @@ virIdentityPtr virIdentityGetSystem(void)
     }
 #endif
 
- cleanup:
-    VIR_FREE(username);
-    VIR_FREE(groupname);
-    return ret;
-
  error:
     virObjectUnref(ret);
-    ret = NULL;
-    goto cleanup;
+    return NULL;
 }
 
 
@@ -461,15 +455,14 @@ int virIdentitySetUNIXUserName(virIdentityPtr ident,
 int virIdentitySetUNIXUserID(virIdentityPtr ident,
                              uid_t uid)
 {
-    char *val;
-    int ret;
+    VIR_AUTOFREE(char *) val = NULL;
+
     if (virAsprintf(&val, "%d", (int)uid) < 0)
         return -1;
-    ret = virIdentitySetAttr(ident,
+
+    return virIdentitySetAttr(ident,
                              VIR_IDENTITY_ATTR_UNIX_USER_ID,
                              val);
-    VIR_FREE(val);
-    return ret;
 }
 
 
@@ -485,45 +478,42 @@ int virIdentitySetUNIXGroupName(virIdentityPtr ident,
 int virIdentitySetUNIXGroupID(virIdentityPtr ident,
                               gid_t gid)
 {
-    char *val;
-    int ret;
+    VIR_AUTOFREE(char *) val = NULL;
+
     if (virAsprintf(&val, "%d", (int)gid) < 0)
         return -1;
-    ret = virIdentitySetAttr(ident,
+
+    return virIdentitySetAttr(ident,
                              VIR_IDENTITY_ATTR_UNIX_GROUP_ID,
                              val);
-    VIR_FREE(val);
-    return ret;
 }
 
 
 int virIdentitySetUNIXProcessID(virIdentityPtr ident,
                                 pid_t pid)
 {
-    char *val;
-    int ret;
+    VIR_AUTOFREE(char *) val = NULL;
+
     if (virAsprintf(&val, "%lld", (long long) pid) < 0)
         return -1;
-    ret = virIdentitySetAttr(ident,
+
+    return virIdentitySetAttr(ident,
                              VIR_IDENTITY_ATTR_UNIX_PROCESS_ID,
                              val);
-    VIR_FREE(val);
-    return ret;
 }
 
 
 int virIdentitySetUNIXProcessTime(virIdentityPtr ident,
                                   unsigned long long timestamp)
 {
-    char *val;
-    int ret;
+    VIR_AUTOFREE(char *) val = NULL;
+
     if (virAsprintf(&val, "%llu", timestamp) < 0)
         return -1;
-    ret = virIdentitySetAttr(ident,
+
+    return virIdentitySetAttr(ident,
                              VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME,
                              val);
-    VIR_FREE(val);
-    return ret;
 }
 
 
-- 
1.8.3.1

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

On Fri, Jun 08, 2018 at 01:04:40AM +0530, Sukrit Bhatnagar wrote:
> By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
> macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.

You're missing the SoB line here. Make sure to add that to comply with the
Developer Certificate of Origin, for more info, see
https://libvirt.org/governance.html#contributors

> ---
>  src/util/viridentity.c | 54 ++++++++++++++++++++------------------------------
>  1 file changed, 22 insertions(+), 32 deletions(-)
>
> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 2f4307b..2060dd7 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c
> @@ -133,8 +133,8 @@ int virIdentitySetCurrent(virIdentityPtr ident)
>   */
>  virIdentityPtr virIdentityGetSystem(void)
>  {
> -    char *username = NULL;
> -    char *groupname = NULL;
> +    VIR_AUTOFREE(char *) username = NULL;
> +    VIR_AUTOFREE(char *) groupname = NULL;
>      unsigned long long startTime;
>      virIdentityPtr ret = NULL;
>  #if WITH_SELINUX
> @@ -154,14 +154,14 @@ virIdentityPtr virIdentityGetSystem(void)
>          goto error;
>
>      if (!(username = virGetUserName(geteuid())))
> -        goto cleanup;
> +        return ret;
>      if (virIdentitySetUNIXUserName(ret, username) < 0)
>          goto error;
>      if (virIdentitySetUNIXUserID(ret, getuid()) < 0)
>          goto error;
>
>      if (!(groupname = virGetGroupName(getegid())))
> -        goto cleanup;
> +        return ret;
>      if (virIdentitySetUNIXGroupName(ret, groupname) < 0)
>          goto error;
>      if (virIdentitySetUNIXGroupID(ret, getgid()) < 0)
> @@ -172,7 +172,7 @@ virIdentityPtr virIdentityGetSystem(void)
>          if (getcon(&con) < 0) {
>              virReportSystemError(errno, "%s",
>                                   _("Unable to lookup SELinux process context"));
> -            goto cleanup;
> +            return ret;
>          }
>          if (virIdentitySetSELinuxContext(ret, con) < 0) {
>              freecon(con);
> @@ -182,15 +182,9 @@ virIdentityPtr virIdentityGetSystem(void)
>      }
>  #endif
>
> - cleanup:
> -    VIR_FREE(username);
> -    VIR_FREE(groupname);
> -    return ret;
> -
>   error:
>      virObjectUnref(ret);
> -    ret = NULL;
> -    goto cleanup;
> +    return NULL;

you're returning NULL on success too, which causes the test suite to fail, make
sure that you run make check -j X && make syntax-check -j X after every patch
in the series.

X - number of logical CPUs + 1

I briefly went through the other VIR_AUTOFREE patches and didn't spot any
problems, I'll have a better look once you post another version of this series
due to the points I raised.

Regards,
Erik

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