Chen Hanxiao
2015-Jun-15 03:49 UTC
[Libguestfs] [PATCH v3 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 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 | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) -- 2.1.0
Chen Hanxiao
2015-Jun-15 03:49 UTC
[Libguestfs] [PATCH v3 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> --- v3: don't return internal tmp values. daemon/btrfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 39392f7..5011ec4 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -1254,7 +1254,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) { @@ -1320,14 +1320,14 @@ do_btrfs_qgroup_show (const char *path) goto error; } *p = '\0'; - this->btrfsqgroup_id = line; + this->btrfsqgroup_id = strdup (line); + if (this->btrfsqgroup_id == NULL) + goto error; } - free (lines); return ret; error: - free_stringslen (lines, nr_qgroups + 2); if (ret) free (ret->guestfs_int_btrfsqgroup_list_val); free (ret); -- 2.1.0
Chen Hanxiao
2015-Jun-15 03:49 UTC
[Libguestfs] [PATCH v3 2/3] do_btrfs_subvolume_list: fix a bad return value
don't return a value which is to be freed. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- v3: don't return internal tmp values. daemon/btrfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/btrfs.c b/daemon/btrfs.c index 5011ec4..c1462fd 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]; @@ -531,16 +531,16 @@ 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 = strdup (line); + 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); free (ret); if (re) pcre_free (re); -- 2.1.0
Chen Hanxiao
2015-Jun-15 03:49 UTC
[Libguestfs] [PATCH v2 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 c1462fd..5d87f53 100644 --- a/daemon/btrfs.c +++ b/daemon/btrfs.c @@ -1704,10 +1704,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; @@ -1830,10 +1830,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-16 13:27 UTC
Re: [Libguestfs] [PATCH v3 1/3] do_btrfs_qgroup_show: fix a bad return value
On Monday 15 June 2015 11:49:35 Chen Hanxiao wrote:> 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> > --- > v3: don't return internal tmp values. > > daemon/btrfs.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 39392f7..5011ec4 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -1254,7 +1254,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) { > @@ -1320,14 +1320,14 @@ do_btrfs_qgroup_show (const char *path)The full code here is: if (sscanf (line, "%" SCNu64 "/%" SCNu64 " %" SCNu64 " %" SCNu64, &dummy1, &dummy2, &this->btrfsqgroup_rfer, &this->btrfsqgroup_excl) != 4) { 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; > + this->btrfsqgroup_id = strdup (line); > + if (this->btrfsqgroup_id == NULL) > + goto error; > }I'd say that sscanf + character class + allocation should be able to extract the string of the qgroup id, so there's no need to change 'line' nor to manually duplicate part of the string. Something like: sscanf (line, "%m[0-9/] %" SCNu64 " %" SCNu64, &this->btrfsqgroup_id, &this->btrfsqgroup_rfer, &this->btrfsqgroup_excl) Thanks, -- Pino Toscano
Pino Toscano
2015-Jun-16 13:30 UTC
Re: [Libguestfs] [PATCH v3 2/3] do_btrfs_subvolume_list: fix a bad return value
On Monday 15 June 2015 11:49:36 Chen Hanxiao wrote:> don't return a value which is to be freed. > > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> > --- > v3: don't return internal tmp values. > > daemon/btrfs.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 5011ec4..c1462fd 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]; > @@ -531,16 +531,16 @@ 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 = strdup (line); > + if (this->btrfssubvolume_path == NULL) > + goto error; > }Instead of moving bytes in 'line', since you know the start of the string and how many characters were matched for it, use strndup directly from the starting byte. Something like: this->btrfssubvolume_path = strndup (line + ovector[6], ovector[7] - ovector[6]); Thanks, -- Pino Toscano
Pino Toscano
2015-Jun-16 13:46 UTC
Re: [Libguestfs] [PATCH v2 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
On Monday 15 June 2015 11:49:37 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 c1462fd..5d87f53 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -1704,10 +1704,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; > @@ -1830,10 +1830,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. -- Pino Toscano
Richard W.M. Jones
2015-Jun-16 16:06 UTC
Re: [Libguestfs] [PATCH v3 0/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
On Mon, Jun 15, 2015 at 11:49:34AM +0800, Chen Hanxiao wrote:> As Pino's comment, we should take advantage of > macro CLEANUP_FREE_STRING_LIST > > v3: fix test case failure > v2: properly initialize linesLooks generally good, but would like to see the simple adjustment suggested by Pino in patch 2. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Seemingly Similar Threads
- [PATCH v2] btrfs: use CLEANUP_FREE_STRING_LIST for list free
- [PATCH] btrfs: use CLEANUP_FREE_STRING_LIST for list free
- [PATCH v4 3/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
- [PATCH v3 0/3] btrfs: use CLEANUP_FREE_STRING_LIST for list free
- [PATCH v3 1/2] New API: btrfs_balance_status