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

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

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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.1.0

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>
---
v3: don't return internal tmp values.

 daemon/btrfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 39392f7..5011ec4 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1254,7 +1254,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) {
@@ -1320,14 +1320,14 @@ do_btrfs_qgroup_show (const char *path)
       goto error;
     }
     *p = '\0';
-    this->btrfsqgroup_id = line;
+    this->btrfsqgroup_id = strdup (line);
+    if (this->btrfsqgroup_id == NULL)
+      goto error;
   }
 
-  free (lines);
   return ret;
 
 error:
-  free_stringslen (lines, nr_qgroups + 2);
   if (ret)
     free (ret->guestfs_int_btrfsqgroup_list_val);
   free (ret);
-- 
2.1.0

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

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
v3: don't return internal tmp values.

 daemon/btrfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 5011ec4..c1462fd 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];
@@ -531,16 +531,16 @@ 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 = strdup (line);
+    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);
   free (ret);
   if (re) pcre_free (re);
-- 
2.1.0

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 c1462fd..5d87f53 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -1704,10 +1704,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;
@@ -1830,10 +1830,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

On Monday 15 June 2015 11:49:35 Chen Hanxiao wrote:
> 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>
> ---
> v3: don't return internal tmp values.
> 
>  daemon/btrfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 39392f7..5011ec4 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -1254,7 +1254,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) {
> @@ -1320,14 +1320,14 @@ do_btrfs_qgroup_show (const char *path)
The full code here is:

    if (sscanf (line, "%" SCNu64 "/%" SCNu64 " %"
SCNu64 " %" SCNu64,
                &dummy1, &dummy2, &this->btrfsqgroup_rfer,
                &this->btrfsqgroup_excl) != 4) {
      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;
> +    this->btrfsqgroup_id = strdup (line);
> +    if (this->btrfsqgroup_id == NULL)
> +      goto error;
>    }
I'd say that sscanf + character class + allocation should be able to
extract the string of the qgroup id, so there's no need to change
'line' nor to manually duplicate part of the string.

Something like:

  sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64,
          &this->btrfsqgroup_id, &this->btrfsqgroup_rfer,
          &this->btrfsqgroup_excl)

Thanks,
-- 
Pino Toscano

On Monday 15 June 2015 11:49:36 Chen Hanxiao wrote:
> don't return a value which is to be freed.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
> v3: don't return internal tmp values.
> 
>  daemon/btrfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index 5011ec4..c1462fd 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];
> @@ -531,16 +531,16 @@ 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 = strdup (line);
> +    if (this->btrfssubvolume_path == NULL)
> +      goto error;
>    }
Instead of moving bytes in 'line', since you know the start of the
string and how many characters were matched for it, use strndup
directly from the starting byte. Something like:

 this->btrfssubvolume_path = 
   strndup (line + ovector[6], ovector[7] - ovector[6]);

Thanks,
-- 
Pino Toscano

On Monday 15 June 2015 11:49:37 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 c1462fd..5d87f53 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -1704,10 +1704,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;
> @@ -1830,10 +1830,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.

-- 
Pino Toscano

On Mon, Jun 15, 2015 at 11:49:34AM +0800, Chen Hanxiao
wrote:
> As Pino's comment, we should take advantage of
> macro CLEANUP_FREE_STRING_LIST
> 
> v3: fix test case failure
> v2: properly initialize lines
Looks generally good, but would like to see the simple adjustment
suggested by Pino in patch 2.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org