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

[Pino Toscano]

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 at 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