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
[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
[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
[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
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
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
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
Reasonably Related Threads
- Re: [PATCH v4 1/3] do_btrfs_qgroup_show: fix a bad return value
- [PATCH v5 1/2] do_btrfs_qgroup_show: fix a bad return value
- [PATCH 09/11] New API: btrfs_qgroup_show
- [PATCH v4 0/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
- [PATCH v3 1/3] do_btrfs_qgroup_show: fix a bad return value