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
Possibly Parallel Threads
- [PATCH v3 0/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
- 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
- Re: [PATCH v4 2/3] do_btrfs_subvolume_list: fix a bad return value
- [PATCH] btrfs: keep calloc and its error message match