[PATCH 00/17] btrfs-progs: More misc fixes & cleanups

[PATCH 00/17] btrfs-progs: More misc fixes & cleanups

[Eric Sandeen]

A bunch of Coverity static analysis checker fixes, including
a couple actual bugfixes.

This gets it down from around 80 defects to about 50; I have a couple
other patches I need to clean up which quiets it even more.

By getting it to a tolerable level, subsequent runs to check for
regressions & new problems should be more manageable...

thanks,
-Eric

[PATCH 01/17] btrfs-progs: Unify size-parsing
[PATCH 02/17] btrfs-progs: fix btrfs_get_subvol cut/paste  error
[PATCH 03/17] btrfs-progs: Remove write-only var fdres in cmd_dev_stats()
[PATCH 04/17] btrfs-progs: btrfs_list_get_path_rootid error handling
[PATCH 05/17] btrfs-progs: avoid double-free in __btrfs_map_block
[PATCH 06/17] btrfs-progs: fix open error test in cmd_start_replace
[PATCH 07/17] btrfs-progs: fix close of error fd in scrub cancel
[PATCH 08/17] btrfs-progs: more scrub cancel error handling
[PATCH 09/17] btrfs-progs: free memory before error exit in read_whole_eb
[PATCH 10/17] btrfs-progs: don't call close on error fd
[PATCH 11/17] btrfs-progs: provide positive errno to strerror in cmd_restore
[PATCH 12/17] btrfs-progs: free allocated di_args in cmd_start_replace
[PATCH 13/17] btrfs-progs: close fd on cmd_subvol_get_default return
[PATCH 14/17] btrfs-progs: fix mem leak in resolve_root
[PATCH 15/17] btrfs-progs: Tidy up resolve_root
[PATCH 16/17] btrfs-progs: fix fd leak in cmd_subvol_set_default
[PATCH 17/17] btrfs-progs: replace strtok_r with strsep

[PATCH 01/17] btrfs-progs: Unify size-parsing

[Eric Sandeen]

cmds-qgroup.c contained a parse_limit() function which
duplicates much of the functionality of parse_size.
The only unique behavior is to handle "none"; then we
can just pass it off to parse_size().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-qgroup.c |   44 ++++++--------------------------------------
 utils.c       |    8 +++++++-
 utils.h       |    2 +-
 3 files changed, 14 insertions(+), 40 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 26f0ab0..ce013c8 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -198,43 +198,13 @@ done:
 	return ret;
 }
 
-static int parse_limit(const char *p, unsigned long long *s)
+static u64 parse_limit(const char *p)
 {
-	char *endptr;
-	unsigned long long size;
-
-	if (strcasecmp(p, "none") == 0) {
-		*s = 0;
-		return 1;
-	}
-	size = strtoull(p, &endptr, 10);
-	switch (*endptr) {
-	case 'T':
-	case 't':
-		size *= 1024;
-	case 'G':
-	case 'g':
-		size *= 1024;
-	case 'M':
-	case 'm':
-		size *= 1024;
-	case 'K':
-	case 'k':
-		size *= 1024;
-		++endptr;
-		break;
-	case 0:
-		break;
-	default:
-		return 0;
-	}
-
-	if (*endptr)
+	if (strcasecmp(p, "none") == 0)
 		return 0;
 
-	*s = size;
-
-	return 1;
+	/* parse_size() will exit() on any error */
+	return parse_size(p);
 }
 
 static const char * const cmd_qgroup_assign_usage[] = {
@@ -364,10 +334,8 @@ static int cmd_qgroup_limit(int argc, char **argv)
 	if (check_argc_min(argc - optind, 2))
 		usage(cmd_qgroup_limit_usage);
 
-	if (!parse_limit(argv[optind], &size)) {
-		fprintf(stderr, "Invalid size argument given\n");
-		return 1;
-	}
+	/* parse_limit will exit on any error */
+	size = parse_limit(argv[optind]);
 
 	memset(&args, 0, sizeof(args));
 	if (size) {
diff --git a/utils.c b/utils.c
index d660507..bc6d5fe 100644
--- a/utils.c
+++ b/utils.c
@@ -1251,7 +1251,7 @@ scan_again:
 	return 0;
 }
 
-u64 parse_size(char *s)
+u64 parse_size(const char *s)
 {
 	int i;
 	char c;
@@ -1268,16 +1268,22 @@ u64 parse_size(char *s)
 		switch (c) {
 		case 'e':
 			mult *= 1024;
+			/* Fallthrough */
 		case 'p':
 			mult *= 1024;
+			/* Fallthrough */
 		case 't':
 			mult *= 1024;
+			/* Fallthrough */
 		case 'g':
 			mult *= 1024;
+			/* Fallthrough */
 		case 'm':
 			mult *= 1024;
+			/* Fallthrough */
 		case 'k':
 			mult *= 1024;
+			/* Fallthrough */
 		case 'b':
 			break;
 		default:
diff --git a/utils.h b/utils.h
index 60a0fea..dcdf475 100644
--- a/utils.h
+++ b/utils.h
@@ -47,7 +47,7 @@ char *pretty_sizes(u64 size);
 int check_label(char *input);
 int get_mountpt(char *dev, char *mntpt, size_t size);
 int btrfs_scan_block_devices(int run_ioctl);
-u64 parse_size(char *s);
+u64 parse_size(const char *s);
 int open_file_or_dir(const char *fname);
 int get_device_info(int fd, u64 devid,
 		    struct btrfs_ioctl_dev_info_args *di_args);
-- 
1.7.1

[PATCH 02/17] btrfs-progs: fix btrfs_get_subvol cut/paste error

[Eric Sandeen]

in btrfs_get_subvol(), there is a cut and paste error:

       if (ri->full_path)
               the_ri->full_path = strdup(ri->full_path);
       else
               the_ri->name = NULL;

It should be setting the_ri->full_path to NULL here.
Do it in a function instead of the cpoy & paste to avoid future
errors.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 btrfs-list.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index d02d620..ab9179f 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1515,6 +1515,13 @@ int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 	return 0;
 }
 
+char *strdup_or_null(const char *s)
+{
+	if (!s)
+		return NULL;
+	return strdup(s);
+}
+
 int btrfs_get_subvol(int fd, struct root_info *the_ri)
 {
 	int ret = 1, rr;
@@ -1537,18 +1544,9 @@ int btrfs_get_subvol(int fd, struct root_info *the_ri)
 		}
 		if (!comp_entry_with_rootid(the_ri, ri, 0)) {
 			memcpy(the_ri, ri, offsetof(struct root_info, path));
-			if (ri->path)
-				the_ri->path = strdup(ri->path);
-			else
-				the_ri->path = NULL;
-			if (ri->name)
-				the_ri->name = strdup(ri->name);
-			else
-				the_ri->name = NULL;
-			if (ri->full_path)
-				the_ri->full_path = strdup(ri->full_path);
-			else
-				the_ri->name = NULL;
+			the_ri->path = strdup_or_null(ri->path);
+			the_ri->name = strdup_or_null(ri->name);
+			the_ri->full_path = strdup_or_null(ri->full_path);
 			ret = 0;
 			break;
 		}
-- 
1.7.1

[PATCH 03/17] btrfs-progs: Remove write-only var fdres in cmd_dev_stats()

[Eric Sandeen]

fdres is initialized to -1, then later tested, but never
set.  Just remove it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-device.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 198ad68..58df6da 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -295,7 +295,6 @@ static int cmd_dev_stats(int argc, char **argv)
 	int fdmnt;
 	int i;
 	char c;
-	int fdres = -1;
 	int err = 0;
 	__u64 flags = 0;
 
@@ -390,8 +389,6 @@ static int cmd_dev_stats(int argc, char **argv)
 out:
 	free(di_args);
 	close(fdmnt);
-	if (fdres > -1)
-		close(fdres);
 
 	return err;
 }
-- 
1.7.1

[PATCH 04/17] btrfs-progs: btrfs_list_get_path_rootid error handling

[Eric Sandeen]

btrfs_list_get_path_rootid() tries to return a negative
number on error, but it's a u64 function.  Callers which test
for a return < 0 will never see an error.

Change the function to fill in the rootid via a pointer,
and then return a simple int as error.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 btrfs-list.c     |   31 +++++++++++++++++++++++--------
 btrfs-list.h     |    2 +-
 cmds-subvolume.c |   14 +++++++++-----
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index ab9179f..851c059 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1500,8 +1500,13 @@ int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 {
 	struct root_lookup root_lookup;
 	struct root_lookup root_sort;
-	int ret;
-	u64 top_id = (full_path ? 0 : btrfs_list_get_path_rootid(fd));
+	int ret = 0;
+	u64 top_id = 0;
+
+	if (full_path)
+		ret = btrfs_list_get_path_rootid(fd, &top_id);
+	if (ret)
+		return ret;
 
 	ret = btrfs_list_subvols(fd, &root_lookup);
 	if (ret)
@@ -1524,13 +1529,18 @@ char *strdup_or_null(const char *s)
 
 int btrfs_get_subvol(int fd, struct root_info *the_ri)
 {
-	int ret = 1, rr;
+	int ret, rr;
 	struct root_lookup rl;
 	struct rb_node *rbn;
 	struct root_info *ri;
-	u64 root_id = btrfs_list_get_path_rootid(fd);
+	u64 root_id;
+
+	ret = btrfs_list_get_path_rootid(fd, &root_id);
+	if (ret)
+		return ret;
 
-	if (btrfs_list_subvols(fd, &rl))
+	ret = btrfs_list_subvols(fd, &rl);
+	if (ret)
 		return ret;
 
 	rbn = rb_first(&rl.root);
@@ -1741,7 +1751,11 @@ char *btrfs_list_path_for_root(int fd, u64 root)
 	struct rb_node *n;
 	char *ret_path = NULL;
 	int ret;
-	u64 top_id = btrfs_list_get_path_rootid(fd);
+	u64 top_id;
+
+	ret = btrfs_list_get_path_rootid(fd, &top_id);
+	if (ret)
+		return ERR_PTR(ret);
 
 	ret = __list_subvol_search(fd, &root_lookup);
 	if (ret < 0)
@@ -1868,7 +1882,7 @@ int btrfs_list_parse_filter_string(char *optarg,
 	return 0;
 }
 
-u64 btrfs_list_get_path_rootid(int fd)
+int btrfs_list_get_path_rootid(int fd, u64 *treeid)
 {
 	int  ret;
 	struct btrfs_ioctl_ino_lookup_args args;
@@ -1883,5 +1897,6 @@ u64 btrfs_list_get_path_rootid(int fd)
 			strerror(errno));
 		return ret;
 	}
-	return args.treeid;
+	*treeid = args.treeid;
+	return 0;
 }
diff --git a/btrfs-list.h b/btrfs-list.h
index 5d87454..2894451 100644
--- a/btrfs-list.h
+++ b/btrfs-list.h
@@ -156,5 +156,5 @@ int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 int btrfs_list_find_updated_files(int fd, u64 root_id, u64 oldest_gen);
 int btrfs_list_get_default_subvolume(int fd, u64 *default_id);
 char *btrfs_list_path_for_root(int fd, u64 root);
-u64 btrfs_list_get_path_rootid(int fd);
+int btrfs_list_get_path_rootid(int fd, u64 *treeid);
 int btrfs_get_subvol(int fd, struct root_info *the_ri);
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index ea128fc..f9258fc 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -433,7 +433,11 @@ static int cmd_subvol_list(int argc, char **argv)
 		goto out;
 	}
 
-	top_id = btrfs_list_get_path_rootid(fd);
+	ret = btrfs_list_get_path_rootid(fd, &top_id);
+	if (ret) {
+		fprintf(stderr, "ERROR: can't get rootid for '%s'\n", subvol);
+		goto out;
+	}
 
 	if (is_list_all)
 		btrfs_list_setup_filter(&filter_set,
@@ -821,8 +825,8 @@ static int cmd_subvol_show(int argc, char **argv)
 		goto out;
 	}
 
-	sv_id = btrfs_list_get_path_rootid(fd);
-	if (sv_id < 0) {
+	ret = btrfs_list_get_path_rootid(fd, &sv_id);
+	if (ret) {
 		fprintf(stderr, "ERROR: can't get rootid for '%s'\n",
 			fullpath);
 		goto out;
@@ -834,8 +838,8 @@ static int cmd_subvol_show(int argc, char **argv)
 		goto out;
 	}
 
-	mntid = btrfs_list_get_path_rootid(mntfd);
-	if (mntid < 0) {
+	ret = btrfs_list_get_path_rootid(mntfd, &mntid);
+	if (ret) {
 		fprintf(stderr, "ERROR: can't get rootid for '%s'\n", mnt);
 		goto out;
 	}
-- 
1.7.1

[PATCH 05/17] btrfs-progs: avoid double-free in __btrfs_map_block

[Eric Sandeen]

__btrfs_map_block() can possibly do the goto again: loop after
having allocated & freed the "multi" pointer.  There are then
a couple error conditions where it will attempt to again kfree
the now non-NULL multi pointer.  So before retrying, reset
multi to NULL after we free it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 volumes.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/volumes.c b/volumes.c
index c8fbde3..ca1b402 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1226,6 +1226,7 @@ again:
 	if (multi_ret && stripes_allocated < stripes_required) {
 		stripes_allocated = stripes_required;
 		kfree(multi);
+		multi = NULL;
 		goto again;
 	}
 	stripe_nr = offset;
-- 
1.7.1

[PATCH 06/17] btrfs-progs: fix open error test in cmd_start_replace

[Eric Sandeen]

open() returns a negative fd on failure, not 0.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-replace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index d14c9b5..9397396 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -235,7 +235,7 @@ static int cmd_start_replace(int argc, char **argv)
 		}
 	} else {
 		fdsrcdev = open(srcdev, O_RDWR);
-		if (!fdsrcdev) {
+		if (fdsrcdev < 0) {
 			fprintf(stderr, "Error: Unable to open device '%s'\n",
 				srcdev);
 			goto leave_with_error;
-- 
1.7.1

[PATCH 07/17] btrfs-progs: fix close of error fd in scrub cancel

[Eric Sandeen]

If we retry opening the mountpoint and fail, we'll call
close on a filehandle w/ value -1.  Rearrange so the
retry uses the same open and same error handling.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-scrub.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index b984e96..353d9cb 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1448,13 +1448,13 @@ static int cmd_scrub_cancel(int argc, char **argv)
 
 	path = argv[1];
 
+again:
 	fdmnt = open_file_or_dir(path);
 	if (fdmnt < 0) {
 		fprintf(stderr, "ERROR: scrub cancel failed\n");
-		return 12;
+		return 1;
 	}
 
-again:
 	ret = ioctl(fdmnt, BTRFS_IOC_SCRUB_CANCEL, NULL);
 	err = errno;
 
@@ -1463,13 +1463,10 @@ again:
 		ret = check_mounted_where(fdmnt, path, mp, sizeof(mp),
 					  &fs_devices_mnt);
 		if (ret) {
-			/* It is a device; open the mountpoint. */
+			/* It is a device; try again with the mountpoint. */
 			close(fdmnt);
-			fdmnt = open_file_or_dir(mp);
-			if (fdmnt >= 0) {
-				path = mp;
-				goto again;
-			}
+			path = mp;
+			goto again;
 		}
 	}
 
-- 
1.7.1

[PATCH 08/17] btrfs-progs: more scrub cancel error handling

[Eric Sandeen]

If we request scrub cancel on an unmounted or
non-btrfs device, we still get a "scrub canceled"
success message:

# btrfs scrub cancel /dev/loop1
scrub cancelled
# blkid /dev/loop1
/dev/loop1: UUID="7f586941-1d5e-4ba7-9caa-b35934849957" TYPE="xfs"

Fix this so that if check_mounted_where returns 0
we don't report success.

While we're at it, use perror to report the reason for an open
failure, if we get one.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-scrub.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index 353d9cb..da4120f 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1451,7 +1451,7 @@ static int cmd_scrub_cancel(int argc, char **argv)
 again:
 	fdmnt = open_file_or_dir(path);
 	if (fdmnt < 0) {
-		fprintf(stderr, "ERROR: scrub cancel failed\n");
+		perror("ERROR: scrub cancel failed:");
 		return 1;
 	}
 
@@ -1462,11 +1462,18 @@ again:
 		/* path is not a btrfs mount point.  See if it's a device. */
 		ret = check_mounted_where(fdmnt, path, mp, sizeof(mp),
 					  &fs_devices_mnt);
-		if (ret) {
-			/* It is a device; try again with the mountpoint. */
+		if (ret > 0) {
+			/* It's a mounted btrfs device; retry w/ mountpoint. */
 			close(fdmnt);
 			path = mp;
 			goto again;
+		} else {
+			/* It's not a mounted btrfs device either */
+			fprintf(stderr,
+				"ERROR: %s is not a mounted btrfs device\n",
+				path);
+			ret = 1;
+			err = EINVAL;
 		}
 	}
 
@@ -1474,7 +1481,7 @@ again:
 
 	if (ret) {
 		fprintf(stderr, "ERROR: scrub cancel failed on %s: %s\n", path,
-			err == ENOTCONN ? "not running" : strerror(errno));
+			err == ENOTCONN ? "not running" : strerror(err));
 		return 1;
 	}
 
-- 
1.7.1

[PATCH 09/17] btrfs-progs: free memory before error exit in read_whole_eb

[Eric Sandeen]

Free the memory allocated to "multi" before the error
exit in read_whole_eb().  Set it to NULL after we free
it in the loop to avoid any potential double-free.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 disk-io.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 5aa9aa3..897d0cf 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -198,17 +198,21 @@ static int read_whole_eb(struct btrfs_fs_info *info, struct extent_buffer *eb, i
 				      mirror, NULL);
 		if (ret) {
 			printk("Couldn't map the block %Lu\n", eb->start + offset);
+			kfree(multi);
 			return -EIO;
 		}
 		device = multi->stripes[0].dev;
 
-		if (device->fd == 0)
+		if (device->fd == 0) {
+			kfree(multi);
 			return -EIO;
+		}
 
 		eb->fd = device->fd;
 		device->total_ios++;
 		eb->dev_bytenr = multi->stripes[0].physical;
 		kfree(multi);
+		multi = NULL;
 
 		if (read_len > bytes_left)
 			read_len = bytes_left;
-- 
1.7.1

[PATCH 10/17] btrfs-progs: don't call close on error fd

[Eric Sandeen]

In the error case where fd < 0, close(fd) is the wrong
thing to do.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 btrfs-show-super.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/btrfs-show-super.c b/btrfs-show-super.c
index 3614c52..f587f10 100644
--- a/btrfs-show-super.c
+++ b/btrfs-show-super.c
@@ -97,7 +97,6 @@ int main(int argc, char **argv)
 		fd = open(filename, O_RDONLY, 0666);
 		if (fd < 0) {
 			fprintf(stderr, "Could not open %s\n", filename);
-			close(fd);
 			exit(1);
 		}
 
-- 
1.7.1

[PATCH 11/17] btrfs-progs: provide positive errno to strerror in cmd_restore

[Eric Sandeen]

check_mounted returns a negative errno, so it needs to be flipped
again before passing to strerror.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-restore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-restore.c b/cmds-restore.c
index 12b2188..9385042 100644
--- a/cmds-restore.c
+++ b/cmds-restore.c
@@ -836,7 +836,7 @@ int cmd_restore(int argc, char **argv)
 
 	if ((ret = check_mounted(argv[optind])) < 0) {
 		fprintf(stderr, "Could not check mount status: %s\n",
-			strerror(ret));
+			strerror(-ret));
 		return ret;
 	} else if (ret) {
 		fprintf(stderr, "%s is currently mounted.  Aborting.\n", argv[optind]);
-- 
1.7.1

[PATCH 12/17] btrfs-progs: free allocated di_args in cmd_start_replace

[Eric Sandeen]

We only freed this allocation in error paths, and leaked
a bit when it went out of scope normally.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-replace.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index 9397396..4cc32df 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -228,6 +228,7 @@ static int cmd_start_replace(int argc, char **argv)
 		for (i = 0; i < fi_args.num_devices; i++)
 			if (start_args.start.srcdevid == di_args[i].devid)
 				break;
+		free(di_args);
 		if (i == fi_args.num_devices) {
 			fprintf(stderr, "Error: '%s' is not a valid devid for filesystem '%s'\n",
 				srcdev, path);
-- 
1.7.1

[PATCH 13/17] btrfs-progs: close fd on cmd_subvol_get_default return

[Eric Sandeen]

Without this we leak the fd when we return from the
function.

Also, remove the senseless random return values.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-subvolume.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index f9258fc..0dfaefe 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -624,7 +624,7 @@ static const char * const cmd_subvol_get_default_usage[] = {
 
 static int cmd_subvol_get_default(int argc, char **argv)
 {
-	int fd;
+	int fd = -1;
 	int ret;
 	char *subvol;
 	struct btrfs_list_filter_set *filter_set;
@@ -638,35 +638,36 @@ static int cmd_subvol_get_default(int argc, char **argv)
 	ret = test_issubvolume(subvol);
 	if (ret < 0) {
 		fprintf(stderr, "ERROR: error accessing '%s'\n", subvol);
-		return 12;
+		return 1;
 	}
 	if (!ret) {
 		fprintf(stderr, "ERROR: '%s' is not a subvolume\n", subvol);
-		return 13;
+		return 1;
 	}
 
 	fd = open_file_or_dir(subvol);
 	if (fd < 0) {
 		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
-		return 12;
+		return 1;
 	}
 
 	ret = btrfs_list_get_default_subvolume(fd, &default_id);
 	if (ret) {
 		fprintf(stderr, "ERROR: can't perform the search - %s\n",
 			strerror(errno));
-		return ret;
+		goto out;
 	}
 
+	ret = 1;
 	if (default_id == 0) {
 		fprintf(stderr, "ERROR: 'default' dir item not found\n");
-		return ret;
+		goto out;
 	}
 
 	/* no need to resolve roots if FS_TREE is default */
 	if (default_id == BTRFS_FS_TREE_OBJECTID) {
 		printf("ID 5 (FS_TREE)\n");
-		return ret;
+		goto out;
 	}
 
 	filter_set = btrfs_list_alloc_filter_set();
@@ -684,8 +685,11 @@ static int cmd_subvol_get_default(int argc, char **argv)
 
 	if (filter_set)
 		btrfs_list_free_filter_set(filter_set);
+out:
+	if (fd != -1)
+		close(fd);
 	if (ret)
-		return 19;
+		return 1;
 	return 0;
 }
 
-- 
1.7.1

[PATCH 14/17] btrfs-progs: fix mem leak in resolve_root

[Eric Sandeen]

If we exit with error we must free the allocated memory
to avoid a leak.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 btrfs-list.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 851c059..8c3f84d 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
 		* ref_tree = 0 indicates the subvolumes
 		* has been deleted.
 		*/
-		if (!found->ref_tree)
+		if (!found->ref_tree) {
+			free(full_path);
 			return -ENOENT;
+		}
 		int add_len = strlen(found->path);
 
 		/* room for / and for null */
@@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
 		* subvolume was deleted.
 		*/
 		found = root_tree_search(rl, next);
-		if (!found)
+		if (!found) {
+			free(full_path);
 			return -ENOENT;
+		}
 	}
 
 	ri->full_path = full_path;
-- 
1.7.1

[PATCH 15/17] btrfs-progs: Tidy up resolve_root

[Eric Sandeen]

Whitespace fixes and fix a variable declaration after
code.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 btrfs-list.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 8c3f84d..a748d5e 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -564,15 +564,18 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
 	while (1) {
 		char *tmp;
 		u64 next;
+		int add_len;
+
 		/*
-		* ref_tree = 0 indicates the subvolumes
-		* has been deleted.
-		*/
+		 * ref_tree = 0 indicates the subvolumes
+		 * has been deleted.
+		 */
 		if (!found->ref_tree) {
 			free(full_path);
 			return -ENOENT;
 		}
-		int add_len = strlen(found->path);
+
+		add_len = strlen(found->path);
 
 		/* room for / and for null */
 		tmp = malloc(add_len + 2 + len);
@@ -595,7 +598,7 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
 
 		next = found->ref_tree;
 
-		if (next ==  top_id) {
+		if (next == top_id) {
 			ri->top_id = top_id;
 			break;
 		}
-- 
1.7.1

[PATCH 16/17] btrfs-progs: fix fd leak in cmd_subvol_set_default

[Eric Sandeen]

Rearrange cmd_subvol_set_default() slightly so we
don't have to close the fd on an error return.

While we're at it, fix whitespace & remove magic
return values.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-subvolume.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 0dfaefe..461eed9 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -712,24 +712,25 @@ static int cmd_subvol_set_default(int argc, char **argv)
 	subvolid = argv[1];
 	path = argv[2];
 
+	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
+	if (errno == ERANGE) {
+		fprintf(stderr, "ERROR: invalid tree id (%s)\n", subvolid);
+		return 1;
+	}
+
 	fd = open_file_or_dir(path);
 	if (fd < 0) {
 		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
-		return 12;
+		return 1;
 	}
 
-	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
-	if (errno == ERANGE) {
-		fprintf(stderr, "ERROR: invalid tree id (%s)\n",subvolid);
-		return 30;
-	}
 	ret = ioctl(fd, BTRFS_IOC_DEFAULT_SUBVOL, &objectid);
 	e = errno;
 	close(fd);
-	if( ret < 0 ){
+	if (ret < 0) {
 		fprintf(stderr, "ERROR: unable to set a new default subvolume - %s\n",
 			strerror(e));
-		return 30;
+		return 1;
 	}
 	return 0;
 }
-- 
1.7.1

[PATCH 17/17] btrfs-progs: replace strtok_r with strsep

[Eric Sandeen]

The coverity had a false positive complaining that save_ptr
is uninitialized in the call to strtok_r.

We could initialize it, but Zach points out that just using
strsep is a lot simpler if there's only one delimiter,
so just switch to that.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-balance.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/cmds-balance.c b/cmds-balance.c
index b671e1d..e8b9d90 100644
--- a/cmds-balance.c
+++ b/cmds-balance.c
@@ -67,11 +67,9 @@ static int parse_one_profile(const char *profile, u64 *flags)
 static int parse_profiles(char *profiles, u64 *flags)
 {
 	char *this_char;
-	char *save_ptr;
 
-	for (this_char = strtok_r(profiles, "|", &save_ptr);
-	     this_char != NULL;
-	     this_char = strtok_r(NULL, "|", &save_ptr)) {
+	while ((this_char = strsep(&profiles, "|"))) {
+		printf("got profile %s\n", this_char);
 		if (parse_one_profile(this_char, flags))
 			return 1;
 	}
@@ -136,14 +134,12 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
 {
 	char *this_char;
 	char *value;
-	char *save_ptr;
 
 	if (!filters)
 		return 0;
 
-	for (this_char = strtok_r(filters, ",", &save_ptr);
-	     this_char != NULL;
-	     this_char = strtok_r(NULL, ",", &save_ptr)) {
+	while ((this_char = strsep(&filters , ","))) {
+		printf("got %s\n", this_char);
 		if ((value = strchr(this_char, '=')) != NULL)
 			*value++ = 0;
 		if (!strcmp(this_char, "profiles")) {
-- 
1.7.1

Re: [PATCH 01/17] btrfs-progs: Unify size-parsing

[Zach Brown]

>  		case 'e':
>  			mult *= 1024;
> +			/* Fallthrough */

These comments still annoy me :).  And really, that code kind of annoys
me too.  That's a lot of duplicated code for a mapping of characters to
powers of 1024.

How about.. 

u64 pow_u64(u64 x, unsigned y)
{
	u64 ret = 1;

	while (y--)
		ret *= x;

	return ret;
}

u64 get_mult(char unit)
{
	static char *units = "bkmgtpe";
	char *found = index(units, unit);

	if (found)
		return pow_u64(1024, found - units);
	return 0;
}

Seems like a lot less noise for the same functionality.

- z

Re: [PATCH 01/17] btrfs-progs: Unify size-parsing

[Eric Sandeen]

On 2/25/13 5:26 PM, Zach Brown wrote:
>>  		case 'e':
>>  			mult *= 1024;
>> +			/* Fallthrough */
> 
> These comments still annoy me :).  

it shuts up coverity & other static checkers which are worried about
a missing break...

> And really, that code kind of annoys
> me too.  That's a lot of duplicated code for a mapping of characters to
> powers of 1024.
> 
> How about.. 
> 
> u64 pow_u64(u64 x, unsigned y)
> {
> 	u64 ret = 1;
> 
> 	while (y--)
> 		ret *= x;
> 
> 	return ret;
> }
> 
> u64 get_mult(char unit)
> {
> 	static char *units = "bkmgtpe";
> 	char *found = index(units, unit);
> 
> 	if (found)
> 		return pow_u64(1024, found - units);
> 	return 0;
> }
> 
> Seems like a lot less noise for the same functionality.

If you want to submit that patch as a further cleanup I'd review it ;)

But TBH (going out on a limb crossing Zach here ;) my feeble brain
is easily confused by your clever code.  I'd rather just do the
simple thing simply...

-Eric

> - z
> 

Re: [PATCH 01/17] btrfs-progs: Unify size-parsing

[Zach Brown]

> > These comments still annoy me :).  
> 
> it shuts up coverity & other static checkers which are worried about
> a missing break...

Yeah, I know.  And it's annoying!  So my little brain fart there was an
attempt to use a construct that simply couldn't be confused with
mistakes that'd require comments to clear up.

> If you want to submit that patch as a further cleanup I'd review it ;)

Sure!

> But TBH (going out on a limb crossing Zach here ;) my feeble brain
> is easily confused by your clever code.  I'd rather just do the
> simple thing simply...

Hrmph.  pow() is hardly rocket science :).

- z

Re: [PATCH 14/17] btrfs-progs: fix mem leak in resolve_root

[Shilong Wang]

Hello, Eric

2013/2/26 Eric Sandeen <sandeen@redhat.com>:
> If we exit with error we must free the allocated memory
> to avoid a leak.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  btrfs-list.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/btrfs-list.c b/btrfs-list.c
> index 851c059..8c3f84d 100644
> --- a/btrfs-list.c
> +++ b/btrfs-list.c
> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>                 * ref_tree = 0 indicates the subvolumes
>                 * has been deleted.
>                 */
> -               if (!found->ref_tree)
> +               if (!found->ref_tree) {
> +                       free(full_path);
>                         return -ENOENT;
> +               }
>                 int add_len = strlen(found->path);
>
>                 /* room for / and for null */
> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>                 * subvolume was deleted.
>                 */
>                 found = root_tree_search(rl, next);
> -               if (!found)
> +               if (!found) {
> +                       free(full_path);
>                         return -ENOENT;
> +               }
>         }
>
>         ri->full_path = full_path;
> --
> 1.7.1

I think the patch is wrong;
Here we return ENOENT, it means a subvolume/snapshot deletion happens.
We just filter them in the filter_root, But the free work is done by
the function all_subvolume_free..
so your modification will cause a double free..

Thanks,
Wang

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] btrfs-progs: fix mem leak in resolve_root

[Eric Sandeen]

On 2/25/13 6:36 PM, Shilong Wang wrote:
> Hello, Eric
> 
> 2013/2/26 Eric Sandeen <sandeen@redhat.com>:
>> If we exit with error we must free the allocated memory
>> to avoid a leak.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  btrfs-list.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/btrfs-list.c b/btrfs-list.c
>> index 851c059..8c3f84d 100644
>> --- a/btrfs-list.c
>> +++ b/btrfs-list.c
>> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>>                 * ref_tree = 0 indicates the subvolumes
>>                 * has been deleted.
>>                 */
>> -               if (!found->ref_tree)
>> +               if (!found->ref_tree) {
>> +                       free(full_path);
>>                         return -ENOENT;
>> +               }
>>                 int add_len = strlen(found->path);
>>
>>                 /* room for / and for null */
>> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>>                 * subvolume was deleted.
>>                 */
>>                 found = root_tree_search(rl, next);
>> -               if (!found)
>> +               if (!found) {
>> +                       free(full_path);
>>                         return -ENOENT;
>> +               }
>>         }
>>
>>         ri->full_path = full_path;
>> --
>> 1.7.1
> 
> I think the patch is wrong;
> Here we return ENOENT, it means a subvolume/snapshot deletion happens.
> We just filter them in the filter_root, But the free work is done by
> the function all_subvolume_free..
> so your modification will cause a double free..

Thanks for the review.  I'll admit that when looking at too many of
these static checker reports, sometimes things look obvious when
they are actually subtle, and I've certainly made mistakes before. :)

However, full_path location doesn't seem to be available outside the
scope of this function unless we exit normally and do:

        ri->full_path = full_path;

        return 0;
}

If we exit early at the -ENOENT points, it seems that full_path
is leaked; there are no other references to it.
Am I missing something?

Thanks,
-Eric

> Thanks,
> Wang
> 
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 16/17] btrfs-progs: fix fd leak in cmd_subvol_set_default

[Goffredo Baroncelli]

Hi Eric,

On 02/25/2013 11:54 PM, Eric Sandeen wrote:
> Rearrange cmd_subvol_set_default() slightly so we
> don't have to close the fd on an error return.
> 
> While we're at it, fix whitespace & remove magic
> return values.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  cmds-subvolume.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 0dfaefe..461eed9 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -712,24 +712,25 @@ static int cmd_subvol_set_default(int argc, char **argv)
>  	subvolid = argv[1];
>  	path = argv[2];
>  
> +	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);

Could you replace strtoll() with strtoull() ? Note that:

strtoull("0xffffffffffffffff",0,0)  == 0xffffffffffffffff
strtoull("-1",0,0)  == 0xffffffffffffffff
strtoll("-1",0,0)  == 0xffffffffffffffff
strtoll("0xffffffffffffffff",0,0)  -> ERANGE

> +	if (errno == ERANGE) {

Pay attention that if strtoull() doesn't encounter a problem errno *is
not* touched: this check could catch a previous error. I don't know if
it is an hole in the standard or a bug in the gnu-libc; however I think
that before strtoXll() we should put 'errno = 0;'.

> +		fprintf(stderr, "ERROR: invalid tree id (%s)\n", subvolid);
> +		return 1;
> +	}
> +
>  	fd = open_file_or_dir(path);
>  	if (fd < 0) {
>  		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> -		return 12;
> +		return 1;
>  	}
>  
> -	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
> -	if (errno == ERANGE) {
> -		fprintf(stderr, "ERROR: invalid tree id (%s)\n",subvolid);
> -		return 30;
> -	}
>  	ret = ioctl(fd, BTRFS_IOC_DEFAULT_SUBVOL, &objectid);
>  	e = errno;
>  	close(fd);
> -	if( ret < 0 ){
> +	if (ret < 0) {
>  		fprintf(stderr, "ERROR: unable to set a new default subvolume - %s\n",
>  			strerror(e));
> -		return 30;
> +		return 1;
>  	}
>  	return 0;
>  }


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 17/17] btrfs-progs: replace strtok_r with strsep

[Goffredo Baroncelli]

On 02/25/2013 11:54 PM, Eric Sandeen wrote:
> The coverity had a false positive complaining that save_ptr
> is uninitialized in the call to strtok_r.
> 
> We could initialize it, but Zach points out that just using
> strsep is a lot simpler if there's only one delimiter,
> so just switch to that.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  cmds-balance.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/cmds-balance.c b/cmds-balance.c
> index b671e1d..e8b9d90 100644
> --- a/cmds-balance.c
> +++ b/cmds-balance.c
> @@ -67,11 +67,9 @@ static int parse_one_profile(const char *profile, u64 *flags)
>  static int parse_profiles(char *profiles, u64 *flags)
>  {
>  	char *this_char;
> -	char *save_ptr;
>  
> -	for (this_char = strtok_r(profiles, "|", &save_ptr);
> -	     this_char != NULL;
> -	     this_char = strtok_r(NULL, "|", &save_ptr)) {
> +	while ((this_char = strsep(&profiles, "|"))) {
> +		printf("got profile %s\n", this_char);

In the  original code the printf() doesn't exist. May be this is a
residual of a debugging code ?

>  		if (parse_one_profile(this_char, flags))
>  			return 1;
>  	}
> @@ -136,14 +134,12 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
>  {
>  	char *this_char;
>  	char *value;
> -	char *save_ptr;
>  
>  	if (!filters)
>  		return 0;
>  
> -	for (this_char = strtok_r(filters, ",", &save_ptr);
> -	     this_char != NULL;
> -	     this_char = strtok_r(NULL, ",", &save_ptr)) {
> +	while ((this_char = strsep(&filters , ","))) {
> +		printf("got %s\n", this_char);

Same here

>  		if ((value = strchr(this_char, '=')) != NULL)
>  			*value++ = 0;
>  		if (!strcmp(this_char, "profiles")) {


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 01/17] btrfs-progs: Unify size-parsing

[Goffredo Baroncelli]

On 02/25/2013 11:54 PM, Eric Sandeen wrote:
> cmds-qgroup.c contained a parse_limit() function which
> duplicates much of the functionality of parse_size.
> The only unique behavior is to handle "none"; then we
> can just pass it off to parse_size().
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  cmds-qgroup.c |   44 ++++++--------------------------------------
>  utils.c       |    8 +++++++-
>  utils.h       |    2 +-
>  3 files changed, 14 insertions(+), 40 deletions(-)
> 
> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
> index 26f0ab0..ce013c8 100644
> --- a/cmds-qgroup.c
> +++ b/cmds-qgroup.c
> @@ -198,43 +198,13 @@ done:
>  	return ret;
>  }
>  
> -static int parse_limit(const char *p, unsigned long long *s)
> +static u64 parse_limit(const char *p)
>  {
> -	char *endptr;
> -	unsigned long long size;
> -
> -	if (strcasecmp(p, "none") == 0) {
> -		*s = 0;
> -		return 1;
> -	}
> -	size = strtoull(p, &endptr, 10);
> -	switch (*endptr) {
> -	case 'T':
> -	case 't':
> -		size *= 1024;
> -	case 'G':
> -	case 'g':
> -		size *= 1024;
> -	case 'M':
> -	case 'm':
> -		size *= 1024;
> -	case 'K':
> -	case 'k':
> -		size *= 1024;
> -		++endptr;
> -		break;
> -	case 0:
> -		break;
> -	default:
> -		return 0;
> -	}
> -
> -	if (*endptr)
> +	if (strcasecmp(p, "none") == 0)
>  		return 0;
>  
> -	*s = size;
> -
> -	return 1;
> +	/* parse_size() will exit() on any error */
> +	return parse_size(p);

I don't think that this is a good thing to do: the parse_limit behaviour
is the good one: return an error to the caller instead of exit()-ing.

We should convert the parse_size() to return an error, no to convert
parse_limit to exit(). Of course this is out of the goals of this set of
patches.

>  }
>  
>  static const char * const cmd_qgroup_assign_usage[] = {
> @@ -364,10 +334,8 @@ static int cmd_qgroup_limit(int argc, char **argv)
>  	if (check_argc_min(argc - optind, 2))
>  		usage(cmd_qgroup_limit_usage);
>  
> -	if (!parse_limit(argv[optind], &size)) {
> -		fprintf(stderr, "Invalid size argument given\n");
> -		return 1;
> -	}
> +	/* parse_limit will exit on any error */
> +	size = parse_limit(argv[optind]);
>  
>  	memset(&args, 0, sizeof(args));
>  	if (size) {
> diff --git a/utils.c b/utils.c
> index d660507..bc6d5fe 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1251,7 +1251,7 @@ scan_again:
>  	return 0;
>  }
>  
> -u64 parse_size(char *s)
> +u64 parse_size(const char *s)
>  {
>  	int i;
>  	char c;
> @@ -1268,16 +1268,22 @@ u64 parse_size(char *s)
>  		switch (c) {
>  		case 'e':
>  			mult *= 1024;
> +			/* Fallthrough */
>  		case 'p':
>  			mult *= 1024;
> +			/* Fallthrough */
>  		case 't':
>  			mult *= 1024;
> +			/* Fallthrough */
>  		case 'g':
>  			mult *= 1024;
> +			/* Fallthrough */
>  		case 'm':
>  			mult *= 1024;
> +			/* Fallthrough */
>  		case 'k':
>  			mult *= 1024;
> +			/* Fallthrough */
>  		case 'b':
>  			break;
>  		default:
> diff --git a/utils.h b/utils.h
> index 60a0fea..dcdf475 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -47,7 +47,7 @@ char *pretty_sizes(u64 size);
>  int check_label(char *input);
>  int get_mountpt(char *dev, char *mntpt, size_t size);
>  int btrfs_scan_block_devices(int run_ioctl);
> -u64 parse_size(char *s);
> +u64 parse_size(const char *s);
>  int open_file_or_dir(const char *fname);
>  int get_device_info(int fd, u64 devid,
>  		    struct btrfs_ioctl_dev_info_args *di_args);


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 16/17] btrfs-progs: fix fd leak in cmd_subvol_set_default

[Eric Sandeen]

On 2/26/13 12:46 PM, Goffredo Baroncelli wrote:
> Hi Eric,
> 
> On 02/25/2013 11:54 PM, Eric Sandeen wrote:
>> Rearrange cmd_subvol_set_default() slightly so we
>> don't have to close the fd on an error return.
>>
>> While we're at it, fix whitespace & remove magic
>> return values.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  cmds-subvolume.c |   17 +++++++++--------
>>  1 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>> index 0dfaefe..461eed9 100644
>> --- a/cmds-subvolume.c
>> +++ b/cmds-subvolume.c
>> @@ -712,24 +712,25 @@ static int cmd_subvol_set_default(int argc, char **argv)
>>  	subvolid = argv[1];
>>  	path = argv[2];
>>  
>> +	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
> 
> Could you replace strtoll() with strtoull() ? Note that:
> 
> strtoull("0xffffffffffffffff",0,0)  == 0xffffffffffffffff
> strtoull("-1",0,0)  == 0xffffffffffffffff
> strtoll("-1",0,0)  == 0xffffffffffffffff
> strtoll("0xffffffffffffffff",0,0)  -> ERANGE

Probably a good idea, I think I had noticed that earlier and
then spaced it.  :(

But I figure one functional change per patch is the way to go;
making this other change would probably be best under its own commit;
one to fix the fd leak, and one to fix this issue?

>> +	if (errno == ERANGE) {
> 
> Pay attention that if strtoull() doesn't encounter a problem errno *is
> not* touched: this check could catch a previous error. I don't know if
> it is an hole in the standard or a bug in the gnu-libc; however I think
> that before strtoXll() we should put 'errno = 0;'.

yeah, ugh.  But this problem existed before, correct?  So I think a
separate fix makes sense, do you agree?  Or have I made something
worse here with this change?

Thanks,
-Eric



>> +		fprintf(stderr, "ERROR: invalid tree id (%s)\n", subvolid);
>> +		return 1;
>> +	}
>> +
>>  	fd = open_file_or_dir(path);
>>  	if (fd < 0) {
>>  		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> -		return 12;
>> +		return 1;
>>  	}
>>  
>> -	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
>> -	if (errno == ERANGE) {
>> -		fprintf(stderr, "ERROR: invalid tree id (%s)\n",subvolid);
>> -		return 30;
>> -	}
>>  	ret = ioctl(fd, BTRFS_IOC_DEFAULT_SUBVOL, &objectid);
>>  	e = errno;
>>  	close(fd);
>> -	if( ret < 0 ){
>> +	if (ret < 0) {
>>  		fprintf(stderr, "ERROR: unable to set a new default subvolume - %s\n",
>>  			strerror(e));
>> -		return 30;
>> +		return 1;
>>  	}
>>  	return 0;
>>  }
> 
> 

Re: [PATCH 17/17] btrfs-progs: replace strtok_r with strsep

[Eric Sandeen]

On 2/26/13 12:47 PM, Goffredo Baroncelli wrote:
> On 02/25/2013 11:54 PM, Eric Sandeen wrote:
>> The coverity had a false positive complaining that save_ptr
>> is uninitialized in the call to strtok_r.
>>
>> We could initialize it, but Zach points out that just using
>> strsep is a lot simpler if there's only one delimiter,
>> so just switch to that.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  cmds-balance.c |   12 ++++--------
>>  1 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/cmds-balance.c b/cmds-balance.c
>> index b671e1d..e8b9d90 100644
>> --- a/cmds-balance.c
>> +++ b/cmds-balance.c
>> @@ -67,11 +67,9 @@ static int parse_one_profile(const char *profile, u64 *flags)
>>  static int parse_profiles(char *profiles, u64 *flags)
>>  {
>>  	char *this_char;
>> -	char *save_ptr;
>>  
>> -	for (this_char = strtok_r(profiles, "|", &save_ptr);
>> -	     this_char != NULL;
>> -	     this_char = strtok_r(NULL, "|", &save_ptr)) {
>> +	while ((this_char = strsep(&profiles, "|"))) {
>> +		printf("got profile %s\n", this_char);
> 
> In the  original code the printf() doesn't exist. May be this is a
> residual of a debugging code ?

Argh, yes.  Let me resend, thank you.

-Eric

> 
>>  		if (parse_one_profile(this_char, flags))
>>  			return 1;
>>  	}
>> @@ -136,14 +134,12 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
>>  {
>>  	char *this_char;
>>  	char *value;
>> -	char *save_ptr;
>>  
>>  	if (!filters)
>>  		return 0;
>>  
>> -	for (this_char = strtok_r(filters, ",", &save_ptr);
>> -	     this_char != NULL;
>> -	     this_char = strtok_r(NULL, ",", &save_ptr)) {
>> +	while ((this_char = strsep(&filters , ","))) {
>> +		printf("got %s\n", this_char);
> 
> Same here
> 
>>  		if ((value = strchr(this_char, '=')) != NULL)
>>  			*value++ = 0;
>>  		if (!strcmp(this_char, "profiles")) {
> 
> 

Re: [PATCH 01/17] btrfs-progs: Unify size-parsing

[Eric Sandeen]

On 2/26/13 12:50 PM, Goffredo Baroncelli wrote:
> On 02/25/2013 11:54 PM, Eric Sandeen wrote:
>> cmds-qgroup.c contained a parse_limit() function which
>> duplicates much of the functionality of parse_size.
>> The only unique behavior is to handle "none"; then we
>> can just pass it off to parse_size().
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  cmds-qgroup.c |   44 ++++++--------------------------------------
>>  utils.c       |    8 +++++++-
>>  utils.h       |    2 +-
>>  3 files changed, 14 insertions(+), 40 deletions(-)
>>
>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>> index 26f0ab0..ce013c8 100644
>> --- a/cmds-qgroup.c
>> +++ b/cmds-qgroup.c
>> @@ -198,43 +198,13 @@ done:
>>  	return ret;
>>  }
>>  
>> -static int parse_limit(const char *p, unsigned long long *s)
>> +static u64 parse_limit(const char *p)

...

>> +	/* parse_size() will exit() on any error */
>> +	return parse_size(p);
> 
> I don't think that this is a good thing to do: the parse_limit behaviour
> is the good one: return an error to the caller instead of exit()-ing.
> 
> We should convert the parse_size() to return an error, no to convert
> parse_limit to exit(). Of course this is out of the goals of this set of
> patches.

Hm, fair point.  I could either fix it before this patch, and add
a 0.5/17, or add it to my next set of patches.  What do you think?

Thanks,
-Eric

[PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep

[Eric Sandeen]

The coverity runs had a false positive complaining that save_ptr
is uninitialized in the call to strtok_r.

We could initialize it, but Zach points out that just using
strsep is a lot simpler if there's only one delimiter,
so just switch to that.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Remove accidentally-added debug printfs, thanks Geoffredo!

diff --git a/cmds-balance.c b/cmds-balance.c
index b671e1d..cfbb8eb 100644
--- a/cmds-balance.c
+++ b/cmds-balance.c
@@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags)
 static int parse_profiles(char *profiles, u64 *flags)
 {
 	char *this_char;
-	char *save_ptr;
 
-	for (this_char = strtok_r(profiles, "|", &save_ptr);
-	     this_char != NULL;
-	     this_char = strtok_r(NULL, "|", &save_ptr)) {
+	while ((this_char = strsep(&profiles, "|"))) {
 		if (parse_one_profile(this_char, flags))
 			return 1;
 	}
@@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
 {
 	char *this_char;
 	char *value;
-	char *save_ptr;
 
 	if (!filters)
 		return 0;
 
-	for (this_char = strtok_r(filters, ",", &save_ptr);
-	     this_char != NULL;
-	     this_char = strtok_r(NULL, ",", &save_ptr)) {
+	while ((this_char = strsep(&filters , ","))) {
 		if ((value = strchr(this_char, '=')) != NULL)
 			*value++ = 0;
 		if (!strcmp(this_char, "profiles")) {

Re: [PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep

[Ilya Dryomov]

On Tue, Feb 26, 2013 at 02:20:30PM -0600, Eric Sandeen wrote:
> The coverity runs had a false positive complaining that save_ptr
> is uninitialized in the call to strtok_r.
> 
> We could initialize it, but Zach points out that just using
> strsep is a lot simpler if there's only one delimiter,
> so just switch to that.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: Remove accidentally-added debug printfs, thanks Geoffredo!
> 
> diff --git a/cmds-balance.c b/cmds-balance.c
> index b671e1d..cfbb8eb 100644
> --- a/cmds-balance.c
> +++ b/cmds-balance.c
> @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags)
>  static int parse_profiles(char *profiles, u64 *flags)
>  {
>  	char *this_char;
> -	char *save_ptr;
>  
> -	for (this_char = strtok_r(profiles, "|", &save_ptr);
> -	     this_char != NULL;
> -	     this_char = strtok_r(NULL, "|", &save_ptr)) {
> +	while ((this_char = strsep(&profiles, "|"))) {
>  		if (parse_one_profile(this_char, flags))
>  			return 1;
>  	}
> @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
>  {
>  	char *this_char;
>  	char *value;
> -	char *save_ptr;
>  
>  	if (!filters)
>  		return 0;
>  
> -	for (this_char = strtok_r(filters, ",", &save_ptr);
> -	     this_char != NULL;
> -	     this_char = strtok_r(NULL, ",", &save_ptr)) {
> +	while ((this_char = strsep(&filters , ","))) {

					  ^^^ whitespace


One of the differences between strtok() and strsep() is that the former
allows multiple delimiters between two tokens.  With strsep(), this

btrfs balance -dfoo1=bar1,,,foo2=bar2 <mnt>

fails with error, whereas with strtok() it passes.  I don't have a
strong opinion here (this has been loosely modeled on the way mount(8)
handles -o options), but might it be better to just initialize save_ptr?
(And yes, I know that strsep() is better ;))

Thanks,

		Ilya

Re: [PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep

[Eric Sandeen]

On 2/26/13 2:40 PM, Ilya Dryomov wrote:
> On Tue, Feb 26, 2013 at 02:20:30PM -0600, Eric Sandeen wrote:
>> The coverity runs had a false positive complaining that save_ptr
>> is uninitialized in the call to strtok_r.
>>
>> We could initialize it, but Zach points out that just using
>> strsep is a lot simpler if there's only one delimiter,
>> so just switch to that.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: Remove accidentally-added debug printfs, thanks Geoffredo!
>>
>> diff --git a/cmds-balance.c b/cmds-balance.c
>> index b671e1d..cfbb8eb 100644
>> --- a/cmds-balance.c
>> +++ b/cmds-balance.c
>> @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags)
>>  static int parse_profiles(char *profiles, u64 *flags)
>>  {
>>  	char *this_char;
>> -	char *save_ptr;
>>  
>> -	for (this_char = strtok_r(profiles, "|", &save_ptr);
>> -	     this_char != NULL;
>> -	     this_char = strtok_r(NULL, "|", &save_ptr)) {
>> +	while ((this_char = strsep(&profiles, "|"))) {
>>  		if (parse_one_profile(this_char, flags))
>>  			return 1;
>>  	}
>> @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
>>  {
>>  	char *this_char;
>>  	char *value;
>> -	char *save_ptr;
>>  
>>  	if (!filters)
>>  		return 0;
>>  
>> -	for (this_char = strtok_r(filters, ",", &save_ptr);
>> -	     this_char != NULL;
>> -	     this_char = strtok_r(NULL, ",", &save_ptr)) {
>> +	while ((this_char = strsep(&filters , ","))) {
> 
> 					  ^^^ whitespace
> 
> 
> One of the differences between strtok() and strsep() is that the former
> allows multiple delimiters between two tokens.  With strsep(), this
> 
> btrfs balance -dfoo1=bar1,,,foo2=bar2 <mnt>
> 
> fails with error, whereas with strtok() it passes.  I don't have a
> strong opinion here (this has been loosely modeled on the way mount(8)
> handles -o options), but might it be better to just initialize save_ptr?
> (And yes, I know that strsep() is better ;))

I don't really care much either way, TBH.  Initializing it seems a little
bit magic, but with a comment as to why, it'd be fine.  If you did it this
way intentionally to allow the above format, and changing it would break
expectations, then I'll happily just initialize save_ptr.

(And, I realize that lots of these changes are pedantic and seemingly
pointless, but we've gotten the static checker errors down from over 100
to under 30 and dropping; the more noise we remove the more likely we are
to pay attention to the output and catch actual errors.  At least that's
my feeling; if people think this is getting to be pointless churn, I'm
ok with that, too).

Thanks for the review,
-Eric

p.s. Zach made me do it. ;)


> Thanks,
> 
> 		Ilya
> 

Re: [PATCH 16/17] btrfs-progs: fix fd leak in cmd_subvol_set_default

[Goffredo Baroncelli]

On 02/26/2013 09:10 PM, Eric Sandeen wrote:
> On 2/26/13 12:46 PM, Goffredo Baroncelli wrote:
>> Hi Eric,
>>
>> On 02/25/2013 11:54 PM, Eric Sandeen wrote:
>>> Rearrange cmd_subvol_set_default() slightly so we
>>> don't have to close the fd on an error return.
>>>
>>> While we're at it, fix whitespace & remove magic
>>> return values.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>  cmds-subvolume.c |   17 +++++++++--------
>>>  1 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>>> index 0dfaefe..461eed9 100644
>>> --- a/cmds-subvolume.c
>>> +++ b/cmds-subvolume.c
>>> @@ -712,24 +712,25 @@ static int cmd_subvol_set_default(int argc, char **argv)
>>>  	subvolid = argv[1];
>>>  	path = argv[2];
>>>  
>>> +	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
>>
>> Could you replace strtoll() with strtoull() ? Note that:
>>
>> strtoull("0xffffffffffffffff",0,0)  == 0xffffffffffffffff
>> strtoull("-1",0,0)  == 0xffffffffffffffff
>> strtoll("-1",0,0)  == 0xffffffffffffffff
>> strtoll("0xffffffffffffffff",0,0)  -> ERANGE
> 
> Probably a good idea, I think I had noticed that earlier and
> then spaced it.  :(
> 
> But I figure one functional change per patch is the way to go;
> making this other change would probably be best under its own commit;
> one to fix the fd leak, and one to fix this issue?

IMHO this would be simple enough to be done in one shot. However this
problem exists also in other points.
May be that for now your patch is ok. But then we should start another
set of patches which correct/sanitise all these use of
"parse_size/strto[u]ll/parse_limit...".

Unfortunately this means that these next series of patches will start
only when these one will be accepted in order to avoid patches conflict.

> 
>>> +	if (errno == ERANGE) {
>>
>> Pay attention that if strtoull() doesn't encounter a problem errno *is
>> not* touched: this check could catch a previous error. I don't know if
>> it is an hole in the standard or a bug in the gnu-libc; however I think
>> that before strtoXll() we should put 'errno = 0;'.
> 
> yeah, ugh.  But this problem existed before, correct?  So I think a
> separate fix makes sense, do you agree?  Or have I made something
> worse here with this change?

No the things aren't worse. You are doing a great work

> 
> Thanks,
> -Eric
> 
> 
> 
>>> +		fprintf(stderr, "ERROR: invalid tree id (%s)\n", subvolid);
>>> +		return 1;
>>> +	}
>>> +
>>>  	fd = open_file_or_dir(path);
>>>  	if (fd < 0) {
>>>  		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>>> -		return 12;
>>> +		return 1;
>>>  	}
>>>  
>>> -	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
>>> -	if (errno == ERANGE) {
>>> -		fprintf(stderr, "ERROR: invalid tree id (%s)\n",subvolid);
>>> -		return 30;
>>> -	}
>>>  	ret = ioctl(fd, BTRFS_IOC_DEFAULT_SUBVOL, &objectid);
>>>  	e = errno;
>>>  	close(fd);
>>> -	if( ret < 0 ){
>>> +	if (ret < 0) {
>>>  		fprintf(stderr, "ERROR: unable to set a new default subvolume - %s\n",
>>>  			strerror(e));
>>> -		return 30;
>>> +		return 1;
>>>  	}
>>>  	return 0;
>>>  }
>>
>>
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 17/17 V2] btrfs-progs: replace strtok_r with strsep

[Ilya Dryomov]

On Tue, Feb 26, 2013 at 02:46:30PM -0600, Eric Sandeen wrote:
> On 2/26/13 2:40 PM, Ilya Dryomov wrote:
> > On Tue, Feb 26, 2013 at 02:20:30PM -0600, Eric Sandeen wrote:
> >> The coverity runs had a false positive complaining that save_ptr
> >> is uninitialized in the call to strtok_r.
> >>
> >> We could initialize it, but Zach points out that just using
> >> strsep is a lot simpler if there's only one delimiter,
> >> so just switch to that.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> V2: Remove accidentally-added debug printfs, thanks Geoffredo!
> >>
> >> diff --git a/cmds-balance.c b/cmds-balance.c
> >> index b671e1d..cfbb8eb 100644
> >> --- a/cmds-balance.c
> >> +++ b/cmds-balance.c
> >> @@ -67,11 +67,8 @@ static int parse_one_profile(const char *profile, u64 *flags)
> >>  static int parse_profiles(char *profiles, u64 *flags)
> >>  {
> >>  	char *this_char;
> >> -	char *save_ptr;
> >>  
> >> -	for (this_char = strtok_r(profiles, "|", &save_ptr);
> >> -	     this_char != NULL;
> >> -	     this_char = strtok_r(NULL, "|", &save_ptr)) {
> >> +	while ((this_char = strsep(&profiles, "|"))) {
> >>  		if (parse_one_profile(this_char, flags))
> >>  			return 1;
> >>  	}
> >> @@ -136,14 +133,11 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
> >>  {
> >>  	char *this_char;
> >>  	char *value;
> >> -	char *save_ptr;
> >>  
> >>  	if (!filters)
> >>  		return 0;
> >>  
> >> -	for (this_char = strtok_r(filters, ",", &save_ptr);
> >> -	     this_char != NULL;
> >> -	     this_char = strtok_r(NULL, ",", &save_ptr)) {
> >> +	while ((this_char = strsep(&filters , ","))) {
> > 
> > 					  ^^^ whitespace
> > 
> > 
> > One of the differences between strtok() and strsep() is that the former
> > allows multiple delimiters between two tokens.  With strsep(), this
> > 
> > btrfs balance -dfoo1=bar1,,,foo2=bar2 <mnt>
> > 
> > fails with error, whereas with strtok() it passes.  I don't have a
> > strong opinion here (this has been loosely modeled on the way mount(8)
> > handles -o options), but might it be better to just initialize save_ptr?
> > (And yes, I know that strsep() is better ;))
> 
> I don't really care much either way, TBH.  Initializing it seems a little
> bit magic, but with a comment as to why, it'd be fine.  If you did it this
> way intentionally to allow the above format, and changing it would break
> expectations, then I'll happily just initialize save_ptr.

Yeah, although I doubt anybody would notice, it's probably better to
keep the old behaviour.

> 
> (And, I realize that lots of these changes are pedantic and seemingly
> pointless, but we've gotten the static checker errors down from over 100
> to under 30 and dropping; the more noise we remove the more likely we are
> to pay attention to the output and catch actual errors.  At least that's
> my feeling; if people think this is getting to be pointless churn, I'm
> ok with that, too).

Not at all.  Btrfs-progs had rarely seen any cleanups, with you and Zach
patching it up and David's integration work it has gotten a whole new
life ;)  BTW, huge thanks for the kernel-style build output, I am
forever grateful..

Thanks,

		Ilya

Re: [PATCH 01/17] btrfs-progs: Unify size-parsing

[Goffredo Baroncelli]

On 02/26/2013 09:17 PM, Eric Sandeen wrote:
> On 2/26/13 12:50 PM, Goffredo Baroncelli wrote:
>> On 02/25/2013 11:54 PM, Eric Sandeen wrote:
>>> cmds-qgroup.c contained a parse_limit() function which
>>> duplicates much of the functionality of parse_size.
>>> The only unique behavior is to handle "none"; then we
>>> can just pass it off to parse_size().
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>  cmds-qgroup.c |   44 ++++++--------------------------------------
>>>  utils.c       |    8 +++++++-
>>>  utils.h       |    2 +-
>>>  3 files changed, 14 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>>> index 26f0ab0..ce013c8 100644
>>> --- a/cmds-qgroup.c
>>> +++ b/cmds-qgroup.c
>>> @@ -198,43 +198,13 @@ done:
>>>  	return ret;
>>>  }
>>>  
>>> -static int parse_limit(const char *p, unsigned long long *s)
>>> +static u64 parse_limit(const char *p)
> 
> ...
> 
>>> +	/* parse_size() will exit() on any error */
>>> +	return parse_size(p);
>>
>> I don't think that this is a good thing to do: the parse_limit behaviour
>> is the good one: return an error to the caller instead of exit()-ing.
>>
>> We should convert the parse_size() to return an error, no to convert
>> parse_limit to exit(). Of course this is out of the goals of this set of
>> patches.
> 
> Hm, fair point.  I could either fix it before this patch, and add
> a 0.5/17, or add it to my next set of patches.  What do you think?

I think that there is a general problem about
parse_size/parse_limit/str[x]ll functions... We should address all these
issues with another set of patches. Your set of patches is big enough,
and now we should stabilise them in order to be accepted.

If we agree that it is not good thing to allow parse_limit to call
exit(), I will leave parse_limit() as is.
If you are brave enough ( :) )  add the /* Fallthrough */ comment and
add the peta and exa cases.

In the future we could move parse_limit in utils.c then implement
parse_size in terms of parse_limits and finally replace all the
occurrences of strto[x]ll...

> 
> Thanks,
> -Eric
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

[PATCH 17/17 V3] btrfs-progs: initialize save_ptr prior to strtok_r

[Eric Sandeen]

The coverity runs had a false positive complaining that
save_ptr is uninitialized in the call to strtok_r.

Turns out that under the covers glibc was doing enough
to confuse the checker about what was being called.

Just to keep the noise down, do a harmless initialization,
with a comment as to why.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V3: Keep strtok_r for old compat, and just init the var.

diff --git a/cmds-balance.c b/cmds-balance.c
index b671e1d..f5dc317 100644
--- a/cmds-balance.c
+++ b/cmds-balance.c
@@ -67,7 +67,7 @@ static int parse_one_profile(const char *profile, u64 *flags)
 static int parse_profiles(char *profiles, u64 *flags)
 {
 	char *this_char;
-	char *save_ptr;
+	char *save_ptr = NULL; /* Satisfy static checkers */
 
 	for (this_char = strtok_r(profiles, "|", &save_ptr);
 	     this_char != NULL;
@@ -136,7 +136,7 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args)
 {
 	char *this_char;
 	char *value;
-	char *save_ptr;
+	char *save_ptr = NULL; /* Satisfy static checkers */
 
 	if (!filters)
 		return 0;

Re: [PATCH 16/17] btrfs-progs: fix fd leak in cmd_subvol_set_default

[David Sterba]

On Tue, Feb 26, 2013 at 10:04:04PM +0100, Goffredo Baroncelli wrote:
> On 02/26/2013 09:10 PM, Eric Sandeen wrote:
> IMHO this would be simple enough to be done in one shot. However this
> problem exists also in other points.
> May be that for now your patch is ok. But then we should start another
> set of patches which correct/sanitise all these use of
> "parse_size/strto[u]ll/parse_limit...".
> 
> Unfortunately this means that these next series of patches will start
> only when these one will be accepted in order to avoid patches conflict.

The small and localized changes as can be found in this series are going
to the integration branches early. Thanks all who are reviewing, it
helps to speed up the process. I prefer to separate the changes here, as
you point out there are other places to be fixed.

thanks,
david

Re: [PATCH 14/17] btrfs-progs: fix mem leak in resolve_root

[David Sterba]

On Mon, Feb 25, 2013 at 10:36:41PM -0600, Eric Sandeen wrote:
> On 2/25/13 6:36 PM, Shilong Wang wrote:
> >> --- a/btrfs-list.c
> >> +++ b/btrfs-list.c
> >> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
> >>                 * ref_tree = 0 indicates the subvolumes
> >>                 * has been deleted.
> >>                 */
> >> -               if (!found->ref_tree)
> >> +               if (!found->ref_tree) {
> >> +                       free(full_path);
> >>                         return -ENOENT;
> >> +               }
> >>                 int add_len = strlen(found->path);
> >>
> >>                 /* room for / and for null */
> >> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
> >>                 * subvolume was deleted.
> >>                 */
> >>                 found = root_tree_search(rl, next);
> >> -               if (!found)
> >> +               if (!found) {
> >> +                       free(full_path);
> >>                         return -ENOENT;
> >> +               }
> >>         }
> >>
> >>         ri->full_path = full_path;
> >> --
> >> 1.7.1
> > 
> > I think the patch is wrong;
> > Here we return ENOENT, it means a subvolume/snapshot deletion happens.
> > We just filter them in the filter_root, But the free work is done by
> > the function all_subvolume_free..
> > so your modification will cause a double free..
> 
> Thanks for the review.  I'll admit that when looking at too many of
> these static checker reports, sometimes things look obvious when
> they are actually subtle, and I've certainly made mistakes before. :)
> 
> However, full_path location doesn't seem to be available outside the
> scope of this function unless we exit normally and do:
> 
>         ri->full_path = full_path;
> 
>         return 0;
> }
> 
> If we exit early at the -ENOENT points, it seems that full_path
> is leaked; there are no other references to it.

I agree with you, the freed value is local.

david

Re: [PATCH 14/17] btrfs-progs: fix mem leak in resolve_root

[Shilong Wang]

Hello, Eric, David

2013/2/27 David Sterba <dsterba@suse.cz>:
> On Mon, Feb 25, 2013 at 10:36:41PM -0600, Eric Sandeen wrote:
>> On 2/25/13 6:36 PM, Shilong Wang wrote:
>> >> --- a/btrfs-list.c
>> >> +++ b/btrfs-list.c
>> >> @@ -568,8 +568,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>> >>                 * ref_tree = 0 indicates the subvolumes
>> >>                 * has been deleted.
>> >>                 */
>> >> -               if (!found->ref_tree)
>> >> +               if (!found->ref_tree) {
>> >> +                       free(full_path);
>> >>                         return -ENOENT;
>> >> +               }
>> >>                 int add_len = strlen(found->path);
>> >>
>> >>                 /* room for / and for null */
>> >> @@ -612,8 +614,10 @@ static int resolve_root(struct root_lookup *rl, struct root_info *ri,
>> >>                 * subvolume was deleted.
>> >>                 */
>> >>                 found = root_tree_search(rl, next);
>> >> -               if (!found)
>> >> +               if (!found) {
>> >> +                       free(full_path);
>> >>                         return -ENOENT;
>> >> +               }
>> >>         }
>> >>
>> >>         ri->full_path = full_path;
>> >> --
>> >> 1.7.1
>> >
>> > I think the patch is wrong;
>> > Here we return ENOENT, it means a subvolume/snapshot deletion happens.
>> > We just filter them in the filter_root, But the free work is done by
>> > the function all_subvolume_free..
>> > so your modification will cause a double free..
>>
>> Thanks for the review.  I'll admit that when looking at too many of
>> these static checker reports, sometimes things look obvious when
>> they are actually subtle, and I've certainly made mistakes before. :)
>>
>> However, full_path location doesn't seem to be available outside the
>> scope of this function unless we exit normally and do:
>>
>>         ri->full_path = full_path;
>>
>>         return 0;
>> }
>>
>> If we exit early at the -ENOENT points, it seems that full_path
>> is leaked; there are no other references to it.

I looked the code carefully, i was wrong before..
Agree with the patch

Thanks,
Wang
>
> I agree with you, the freed value is local.
>
> david

Re: [PATCH 00/17] btrfs-progs: More misc fixes & cleanups

[David Sterba]

On Mon, Feb 25, 2013 at 04:54:33PM -0600, Eric Sandeen wrote:
> A bunch of Coverity static analysis checker fixes, including
> a couple actual bugfixes.
> 
> This gets it down from around 80 defects to about 50; I have a couple
> other patches I need to clean up which quiets it even more.
> 
> By getting it to a tolerable level, subsequent runs to check for
> regressions & new problems should be more manageable...

Thank you very much.

> [PATCH 01/17] btrfs-progs: Unify size-parsing

I've added 02-17 to the queue and left out 01 as it looks like it needs
a few changes after the feedback.

david