Re: [libvirt] [PATCH v1 28/32] util: pidfile: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 28, 2018 at 11:31:43PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
...

>
> @@ -219,11 +199,11 @@ int virPidFileReadPathIfAlive(const char *path,
>  {
>  int ret;
>  bool isLink;
> -char *procPath = NULL;
> -char *procLink = NULL;
> +VIR_AUTOFREE(char *) procPath = NULL;
> +VIR_AUTOFREE(char *) procLink = NULL;
> +VIR_AUTOFREE(char *) resolvedBinPath = NULL;
> +VIR_AUTOFREE(char *) resolvedProcLink = NULL;

I'll move ^this at the end of the section.

Reviewed-by: Erik Skultety 

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

[libvirt] [PATCH v1 28/32] util: pidfile: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
---
 src/util/virpidfile.c | 185 --
 1 file changed, 59 insertions(+), 126 deletions(-)

diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 1a85d43..d82bf92 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -97,29 +97,18 @@ int virPidFileWrite(const char *dir,
 const char *name,
 pid_t pid)
 {
-int rc;
-char *pidfile = NULL;
+VIR_AUTOFREE(char *) pidfile = NULL;
 
-if (name == NULL || dir == NULL) {
-rc = -EINVAL;
-goto cleanup;
-}
+if (name == NULL || dir == NULL)
+return -EINVAL;
 
-if (virFileMakePath(dir) < 0) {
-rc = -errno;
-goto cleanup;
-}
+if (virFileMakePath(dir) < 0)
+return -errno;
 
-if (!(pidfile = virPidFileBuildPath(dir, name))) {
-rc = -ENOMEM;
-goto cleanup;
-}
+if (!(pidfile = virPidFileBuildPath(dir, name)))
+return -ENOMEM;
 
-rc = virPidFileWritePath(pidfile, pid);
-
- cleanup:
-VIR_FREE(pidfile);
-return rc;
+return virPidFileWritePath(pidfile, pid);
 }
 
 
@@ -170,25 +159,16 @@ int virPidFileRead(const char *dir,
const char *name,
pid_t *pid)
 {
-int rc;
-char *pidfile = NULL;
+VIR_AUTOFREE(char *) pidfile = NULL;
 *pid = 0;
 
-if (name == NULL || dir == NULL) {
-rc = -EINVAL;
-goto cleanup;
-}
+if (name == NULL || dir == NULL)
+return -EINVAL;
 
-if (!(pidfile = virPidFileBuildPath(dir, name))) {
-rc = -ENOMEM;
-goto cleanup;
-}
+if (!(pidfile = virPidFileBuildPath(dir, name)))
+return -ENOMEM;
 
-rc = virPidFileReadPath(pidfile, pid);
-
- cleanup:
-VIR_FREE(pidfile);
-return rc;
+return virPidFileReadPath(pidfile, pid);
 }
 
 
@@ -219,11 +199,11 @@ int virPidFileReadPathIfAlive(const char *path,
 {
 int ret;
 bool isLink;
-char *procPath = NULL;
-char *procLink = NULL;
+VIR_AUTOFREE(char *) procPath = NULL;
+VIR_AUTOFREE(char *) procLink = NULL;
+VIR_AUTOFREE(char *) resolvedBinPath = NULL;
+VIR_AUTOFREE(char *) resolvedProcLink = NULL;
 size_t procLinkLen;
-char *resolvedBinPath = NULL;
-char *resolvedProcLink = NULL;
 const char deletedText[] = " (deleted)";
 size_t deletedTextLen = strlen(deletedText);
 pid_t retPid;
@@ -232,7 +212,7 @@ int virPidFileReadPathIfAlive(const char *path,
 *pid = -1;
 
 if ((ret = virPidFileReadPath(path, &retPid)) < 0)
-goto cleanup;
+return ret;
 
 #ifndef WIN32
 /* Check that it's still alive.  Safe to skip this sanity check on
@@ -252,13 +232,12 @@ int virPidFileReadPathIfAlive(const char *path,
 goto cleanup;
 }
 
-if (virAsprintf(&procPath, "/proc/%lld/exe", (long long)retPid) < 0) {
-ret = -ENOMEM;
-goto cleanup;
-}
+if (virAsprintf(&procPath, "/proc/%lld/exe", (long long)retPid) < 0)
+return -ENOMEM;
 
 if ((ret = virFileIsLink(procPath)) < 0)
-goto cleanup;
+return ret;
+
 isLink = ret;
 
 if (isLink && virFileLinkPointsTo(procPath, binPath)) {
@@ -275,27 +254,21 @@ int virPidFileReadPathIfAlive(const char *path,
  * "$procpath (deleted)".  Read that link, remove the " (deleted)"
  * part, and see if it has the same canonicalized name as binpath.
  */
-if (!(procLink = areadlink(procPath))) {
-ret = -errno;
-goto cleanup;
-}
+if (!(procLink = areadlink(procPath)))
+return -errno;
+
 procLinkLen = strlen(procLink);
 if (procLinkLen > deletedTextLen)
 procLink[procLinkLen - deletedTextLen] = 0;
 
 if ((ret = virFileResolveAllLinks(binPath, &resolvedBinPath)) < 0)
-goto cleanup;
+return ret;
 if ((ret = virFileResolveAllLinks(procLink, &resolvedProcLink)) < 0)
-goto cleanup;
+return ret;
 
 ret = STREQ(resolvedBinPath, resolvedProcLink) ? 0 : -1;
 
  cleanup:
-VIR_FREE(procPath);
-VIR_FREE(procLink);
-VIR_FREE(resolvedProcLink);
-VIR_FREE(resolvedBinPath);
-
 /* return the originally set pid of -1 unless we proclaim success */
 if (ret == 0)
 *pid = retPid;
@@ -326,24 +299,15 @@ int virPidFileReadIfAlive(const char *dir,
   pid_t *pid,
   const char *binpath)
 {
-int rc = 0;
-char *pidfile = NULL;
+VIR_AUTOFREE(char *) pidfile = NULL;
 
-if (name == NULL || dir == NULL) {
-rc = -EINVAL;
-goto cleanup;
-}
+if (name == NULL || dir == NULL)
+return -EINVAL;
 
-if (!(pidfile = virPidFileBuildPath(dir, name))) {