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

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

> Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com&gt; > --- > 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 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs