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

[Chen, Hanxiao]

> -----Original Message-----
> From: libguestfs-bounces at redhat.com [mailto:libguestfs-bounces at redhat.com] On
> Behalf Of Pino Toscano
> Sent: Wednesday, June 17, 2015 10:28 PM
> To: libguestfs at redhat.com
> Subject: 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/
> 

Fine.

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

OK.
Although it already works fine.

Will changed in next version.

Regards,
- Chen

> >    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
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs