thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume

If this information is useful, please help other people find it:
Share via:

Chen Hanxiao

2015-Jun-17 08:19 UTC

[Libguestfs] [PATCH v4 0/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free

As Pino's comment, we should take advantage of
macro CLEANUP_FREE_STRING_LIST

v4: remove some redundant strdup
v3: fix test case failure
v2: properly initialize lines

Chen Hanxiao (3):
  do_btrfs_qgroup_show: fix a bad return value
  do_btrfs_subvolume_list: fix a bad return value
  btrfs: use CLEANUP_FREE_STRING_LIST for list free

 daemon/btrfs.c | 70 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

-- 
2.1.0

Chen Hanxiao

2015-Jun-17 08:19 UTC

head link

[Libguestfs] [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value

We should not use tmp lines buffer as return value,
for lines buffer will be freed.

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
v4: take advantage of sscanf's '%m'.
v3: fix test case failure

 daemon/btrfs.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 7b14bac..8d03caa 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1249,7 +1249,7 @@ do_btrfs_qgroup_show (const char *path)
   CLEANUP_FREE char *err = NULL;
   CLEANUP_FREE char *out = NULL;
   int r;
-  char **lines;
+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
 
   path_buf = sysroot_path (path);
   if (path_buf == NULL) {
@@ -1275,17 +1275,19 @@ do_btrfs_qgroup_show (const char *path)
   if (!lines)
     return NULL;
 
-  /* line 0 and 1 are:
+  /* Output of `btrfs qgroup show' is like:
+   *
+   *  qgroupid         rfer         excl
+   *  --------         ----         ----
+   *  0/5        9249849344   9249849344
    *
-   * qgroupid rfer          excl
-   * -------- ----          ----
    */
   size_t nr_qgroups = count_strings (lines) - 2;
   guestfs_int_btrfsqgroup_list *ret = NULL;
   ret = malloc (sizeof *ret);
   if (!ret) {
     reply_with_perror ("malloc");
-    goto error;
+    return NULL;
   }
 
   ret->guestfs_int_btrfsqgroup_list_len = nr_qgroups;
@@ -1293,38 +1295,32 @@ do_btrfs_qgroup_show (const char *path)
     calloc (nr_qgroups, sizeof (struct guestfs_int_btrfsqgroup));
   if (ret->guestfs_int_btrfsqgroup_list_val == NULL) {
     reply_with_perror ("malloc");
-    goto error;
+    free (ret);
+    return NULL;
   }
 
   for (i = 0; i < nr_qgroups; ++i) {
     char *line = lines[i + 2];
     struct guestfs_int_btrfsqgroup *this        
&ret->guestfs_int_btrfsqgroup_list_val[i];
-    uint64_t dummy1, dummy2;
-    char *p;
 
-    if (sscanf (line, "%" SCNu64 "/%" SCNu64 " %"
SCNu64 " %" SCNu64,
-                &dummy1, &dummy2, &this->btrfsqgroup_rfer,
-                &this->btrfsqgroup_excl) != 4) {
+    if (sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64,
+                &this->btrfsqgroup_id, &this->btrfsqgroup_rfer,
+                &this->btrfsqgroup_excl) != 3) {
       reply_with_error ("cannot parse output of qgroup show command:
%s", line);
       goto error;
     }
-    p = strchr(line, ' ');
-    if (!p) {
-      reply_with_error ("truncated line: %s", line);
-      goto error;
-    }
-    *p = '\0';
-    this->btrfsqgroup_id = line;
   }
 
-  free (lines);
   return ret;
 
 error:
-  free_stringslen (lines, nr_qgroups + 2);
-  if (ret)
-    free (ret->guestfs_int_btrfsqgroup_list_val);
+  for (i = 0; i < nr_qgroups; ++i) {
+    struct guestfs_int_btrfsqgroup *this +     
&ret->guestfs_int_btrfsqgroup_list_val[i];
+    free (this->btrfsqgroup_id);
+  }
+  free (ret->guestfs_int_btrfsqgroup_list_val);
   free (ret);
 
   return NULL;
-- 
2.1.0

Chen Hanxiao

2015-Jun-17 08:19 UTC

head link

[Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value

don't return a value which is to be freed.

v4: use strndup
v3: v3: fix test case failure
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
 daemon/btrfs.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 8d03caa..5361984 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs)
 guestfs_int_btrfssubvolume_list *
 do_btrfs_subvolume_list (const mountable_t *fs)
 {
-  char **lines;
+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
   size_t i = 0;
   const size_t MAX_ARGS = 64;
   const char *argv[MAX_ARGS];
@@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs)
   ret = malloc (sizeof *ret);
   if (!ret) {
     reply_with_perror ("malloc");
-    goto error;
+    return NULL;
   }
 
   ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes;
@@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs)
     calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume));
   if (ret->guestfs_int_btrfssubvolume_list_val == NULL) {
     reply_with_perror ("malloc");
-    goto error;
+    free (ret);
+    return NULL;
   }
 
   const char *errptr;
@@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs)
 
     #undef XSTRTOU64
 
-    memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1);
-    this->btrfssubvolume_path = line;
+    this->btrfssubvolume_path +      strndup (line + ovector[6], ovector[7]
- ovector[6]);
+    if (this->btrfssubvolume_path == NULL)
+      goto error;
   }
 
-  free (lines);
   pcre_free (re);
 
   return ret;
 
 error:
-  free_stringslen (lines, nr_subvolumes);
-  if (ret) free (ret->guestfs_int_btrfssubvolume_list_val);
+  for (i = 0; i < nr_subvolumes; ++i) {
+    struct guestfs_int_btrfssubvolume *this +     
&ret->guestfs_int_btrfssubvolume_list_val[i];
+    free (this->btrfssubvolume_path);
+  }
+  free (ret->guestfs_int_btrfssubvolume_list_val);
   free (ret);
-  if (re) pcre_free (re);
+
+  if (re)
+    pcre_free (re);
 
   return NULL;
 }
-- 
2.1.0

Chen Hanxiao

2015-Jun-17 08:19 UTC

head link

[Libguestfs] [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
 daemon/btrfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 5361984..88be9ec 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1692,10 +1692,10 @@ do_btrfs_balance_status (const char *path)
   size_t i = 0;
   CLEANUP_FREE char *path_buf = NULL;
   CLEANUP_FREE char *err = NULL;
+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
   char *out;
   int r;
   guestfs_int_btrfsbalance *ret;
-  char **lines;
   size_t nlines;
   const char *errptr;
   int erroffset;
@@ -1817,10 +1817,10 @@ do_btrfs_scrub_status (const char *path)
   size_t i = 0;
   CLEANUP_FREE char *path_buf = NULL;
   CLEANUP_FREE char *err = NULL;
+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
   char *out;
   int r;
   guestfs_int_btrfsscrub *ret;
-  char **lines;
 
   path_buf = sysroot_path (path);
   if (path_buf == NULL) {
-- 
2.1.0

Pino Toscano

2015-Jun-17 09:08 UTC

head link

Re: [Libguestfs] [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free

On Wednesday 17 June 2015 16:19:33 Chen Hanxiao wrote:> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
>  daemon/btrfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 5361984..88be9ec 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -1692,10 +1692,10 @@ do_btrfs_balance_status (const char *path)
>    size_t i = 0;
>    CLEANUP_FREE char *path_buf = NULL;
>    CLEANUP_FREE char *err = NULL;
> +  CLEANUP_FREE_STRING_LIST char **lines = NULL;
>    char *out;
>    int r;
>    guestfs_int_btrfsbalance *ret;
> -  char **lines;
>    size_t nlines;
>    const char *errptr;
>    int erroffset;
> @@ -1817,10 +1817,10 @@ do_btrfs_scrub_status (const char *path)
>    size_t i = 0;
>    CLEANUP_FREE char *path_buf = NULL;
>    CLEANUP_FREE char *err = NULL;
> +  CLEANUP_FREE_STRING_LIST char **lines = NULL;
>    char *out;
>    int r;
>    guestfs_int_btrfsscrub *ret;
> -  char **lines;
>  
>    path_buf = sysroot_path (path);
>    if (path_buf == NULL) {
> 
LGTM, pushed.

Thanks,
-- 
Pino Toscano

Pino Toscano

2015-Jun-17 14:27 UTC

head link

Re: [Libguestfs] [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value

On Wednesday 17 June 2015 16:19:31 Chen Hanxiao wrote:> We should not use tmp lines buffer as return value,
> for lines buffer will be freed.
s/tmp/temporary/
> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
> v4: take advantage of sscanf's '%m'.
> v3: fix test case failure
> 
>  daemon/btrfs.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 7b14bac..8d03caa 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -1249,7 +1249,7 @@ do_btrfs_qgroup_show (const char *path)
>    CLEANUP_FREE char *err = NULL;
>    CLEANUP_FREE char *out = NULL;
>    int r;
> -  char **lines;
> +  CLEANUP_FREE_STRING_LIST char **lines = NULL;
>  
>    path_buf = sysroot_path (path);
>    if (path_buf == NULL) {
> @@ -1275,17 +1275,19 @@ do_btrfs_qgroup_show (const char *path)
>    if (!lines)
>      return NULL;
>  
> -  /* line 0 and 1 are:
> +  /* Output of `btrfs qgroup show' is like:
> +   *
> +   *  qgroupid         rfer         excl
> +   *  --------         ----         ----
> +   *  0/5        9249849344   9249849344
>     *
> -   * qgroupid rfer          excl
> -   * -------- ----          ----
>     */
>    size_t nr_qgroups = count_strings (lines) - 2;
>    guestfs_int_btrfsqgroup_list *ret = NULL;
>    ret = malloc (sizeof *ret);
>    if (!ret) {
>      reply_with_perror ("malloc");
> -    goto error;
> +    return NULL;
>    }
>  
>    ret->guestfs_int_btrfsqgroup_list_len = nr_qgroups;
> @@ -1293,38 +1295,32 @@ do_btrfs_qgroup_show (const char *path)
>      calloc (nr_qgroups, sizeof (struct guestfs_int_btrfsqgroup));
>    if (ret->guestfs_int_btrfsqgroup_list_val == NULL) {
>      reply_with_perror ("malloc");
> -    goto error;
> +    free (ret);
> +    return NULL;
>    }
You don't need the change here, if later in the "error:" label you
keep
the "if (ret)" condition.
>    for (i = 0; i < nr_qgroups; ++i) {
>      char *line = lines[i + 2];
>      struct guestfs_int_btrfsqgroup *this  >       
&ret->guestfs_int_btrfsqgroup_list_val[i];
> -    uint64_t dummy1, dummy2;
> -    char *p;
>  
> -    if (sscanf (line, "%" SCNu64 "/%" SCNu64 "
%" SCNu64 " %" SCNu64,
> -                &dummy1, &dummy2, &this->btrfsqgroup_rfer,
> -                &this->btrfsqgroup_excl) != 4) {
> +    if (sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64,
> +                &this->btrfsqgroup_id,
&this->btrfsqgroup_rfer,
> +                &this->btrfsqgroup_excl) != 3) {
>        reply_with_error ("cannot parse output of qgroup show command:
%s", line);
>        goto error;
>      }
> -    p = strchr(line, ' ');
> -    if (!p) {
> -      reply_with_error ("truncated line: %s", line);
> -      goto error;
> -    }
> -    *p = '\0';
> -    this->btrfsqgroup_id = line;
>    }
>  
> -  free (lines);
>    return ret;
>  
>  error:
> -  free_stringslen (lines, nr_qgroups + 2);
> -  if (ret)
> -    free (ret->guestfs_int_btrfsqgroup_list_val);
> +  for (i = 0; i < nr_qgroups; ++i) {
> +    struct guestfs_int_btrfsqgroup *this > +     
&ret->guestfs_int_btrfsqgroup_list_val[i];
> +    free (this->btrfsqgroup_id);
No need to save "this", just
  free (ret->guestfs_int_btrfsqgroup_list_val[i].btrfsqgroup_id);
should be enough (still well under 80 characters :) ).

Thanks,
-- 
Pino Toscano

Pino Toscano

2015-Jun-17 14:44 UTC

head link

Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value

On Wednesday 17 June 2015 16:19:32 Chen Hanxiao wrote:> don't return a value which is to be freed.
> 
> v4: use strndup
> v3: v3: fix test case failure
> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
>  daemon/btrfs.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 8d03caa..5361984 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -409,7 +409,7 @@ umount (char *fs_buf, const mountable_t *fs)
>  guestfs_int_btrfssubvolume_list *
>  do_btrfs_subvolume_list (const mountable_t *fs)
>  {
> -  char **lines;
> +  CLEANUP_FREE_STRING_LIST char **lines = NULL;
>    size_t i = 0;
>    const size_t MAX_ARGS = 64;
>    const char *argv[MAX_ARGS];
> @@ -472,7 +472,7 @@ do_btrfs_subvolume_list (const mountable_t *fs)
>    ret = malloc (sizeof *ret);
>    if (!ret) {
>      reply_with_perror ("malloc");
> -    goto error;
> +    return NULL;
>    }
>  
>    ret->guestfs_int_btrfssubvolume_list_len = nr_subvolumes;
> @@ -480,7 +480,8 @@ do_btrfs_subvolume_list (const mountable_t *fs)
>      calloc (nr_subvolumes, sizeof (struct guestfs_int_btrfssubvolume));
>    if (ret->guestfs_int_btrfssubvolume_list_val == NULL) {
>      reply_with_perror ("malloc");
> -    goto error;
> +    free (ret);
> +    return NULL;
>    }
You don't need the change here, if later in the "error:" label you
keep
the "if (ret)" condition.
>    const char *errptr;
> @@ -530,20 +531,27 @@ do_btrfs_subvolume_list (const mountable_t *fs)
>  
>      #undef XSTRTOU64
>  
> -    memmove (line, line + ovector[6], ovector[7] - ovector[6] + 1);
> -    this->btrfssubvolume_path = line;
> +    this->btrfssubvolume_path > +      strndup (line + ovector[6],
ovector[7] - ovector[6]);
> +    if (this->btrfssubvolume_path == NULL)
> +      goto error;
>    }
>  
> -  free (lines);
>    pcre_free (re);
>  
>    return ret;
>  
>  error:
> -  free_stringslen (lines, nr_subvolumes);
> -  if (ret) free (ret->guestfs_int_btrfssubvolume_list_val);
> +  for (i = 0; i < nr_subvolumes; ++i) {
> +    struct guestfs_int_btrfssubvolume *this > +     
&ret->guestfs_int_btrfssubvolume_list_val[i];
> +    free (this->btrfssubvolume_path);
No need to save "this", just
  free (ret->guestfs_int_btrfssubvolume_list_val[i].btrfssubvolume_path);
should be enough.

Thanks,
-- 
Pino Toscano

Possibly Parallel Threads

Search for more possibly parallel threads

Libguestfs - Jun 2015 - Re: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value

[Libguestfs] [PATCH v4 0/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free

[Libguestfs] [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value

[Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value

[Libguestfs] [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free

Re: [Libguestfs] [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free

Re: [Libguestfs] [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value

Re: [Libguestfs] [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value

Possibly Parallel Threads