Re: [PATCH 12/14] btrfs-progs: Issue warnings if ioctls fail in sigint handlers

Re: [PATCH 12/14] btrfs-progs: Issue warnings if ioctls fail in sigint handlers

[Zach Brown]

> +	ret = ioctl(cancel_fd, BTRFS_IOC_SCRUB_CANCEL, NULL);
> +	if (ret < 0)
> +		perror("Scrub cancel failed\n");

Probably don't want the extra newline.

- z

Re: [PATCH 14/14] btrfs-progs: Error handling in scrub_progress_cycle() thread

[Zach Brown]

> -	ret = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
> +	ret = -pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
>  	if (ret)
> -		return ERR_PTR(-ret);
> +		goto out;

Am I the only one who finds ret = -pthread_*() pretty odd? :)  (ret = -0
on success.. ok.. I guess..)

I'd have done something like

	err = pthread_*()
	if (err) {
		ret = -err;

so that it looks like the -1/errno pattern that our brains already have
to deal with.

- z

Re: [PATCH 00/14] btrfs-progs: more Coverity cleanups

[Zach Brown]

> Not a lot of real bugfixes here, but a bit better error handling in
> places.  I sent out 2 dumb patches  (elsewhere) on Friday, though,
> so feel free to take a close & skeptical look at these ;)

Minus those two little direct replies I had, these seem fine :).  Thanks
for keeping this work going.

- z

Re: [PATCH 14/14] btrfs-progs: Error handling in scrub_progress_cycle() thread

[Eric Sandeen]

On 3/4/13 4:00 PM, Zach Brown wrote:
>> -	ret = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
>> +	ret = -pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
>>  	if (ret)
>> -		return ERR_PTR(-ret);
>> +		goto out;
> 
> Am I the only one who finds ret = -pthread_*() pretty odd? :)  (ret = -0
> on success.. ok.. I guess..)
> 
> I'd have done something like
> 
> 	err = pthread_*()
> 	if (err) {
> 		ret = -err;
> 
> so that it looks like the -1/errno pattern that our brains already have
> to deal with.
> 
> - z
> 

Yeah, I wasn't thrilled either.  Fair enough.  I'll do that & resend.

-Eric

Re: [PATCH 12/14] btrfs-progs: Issue warnings if ioctls fail in sigint handlers

[Eric Sandeen]

On 3/4/13 3:57 PM, Zach Brown wrote:
>> +	ret = ioctl(cancel_fd, BTRFS_IOC_SCRUB_CANCEL, NULL);
>> +	if (ret < 0)
>> +		perror("Scrub cancel failed\n");
> 
> Probably don't want the extra newline.
> 
> - z
> 

Doh, thanks.

[PATCH 12/14 V2] btrfs-progs: Issue warnings if ioctls fail in sigint handlers

[Eric Sandeen]

The two sigint handlers issue ioctls to clean up, but if
they fail, noone would know.  I'm not sure there is
any other error handling to be done at this point, but a
notification seems wise.

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

diff --git a/cmds-replace.c b/cmds-replace.c
index 4cc32df..10030f6 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -77,10 +77,13 @@ static int is_numerical(const char *str)
 static int dev_replace_cancel_fd = -1;
 static void dev_replace_sigint_handler(int signal)
 {
+	int ret;
 	struct btrfs_ioctl_dev_replace_args args = {0};
 
 	args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_CANCEL;
-	ioctl(dev_replace_cancel_fd, BTRFS_IOC_DEV_REPLACE, &args);
+	ret = ioctl(dev_replace_cancel_fd, BTRFS_IOC_DEV_REPLACE, &args);
+	if (ret < 0)
+		perror("Device replace cancel failed");
 }
 
 static int dev_replace_handle_sigint(int fd)
diff --git a/cmds-scrub.c b/cmds-scrub.c
index da4120f..f73b3c6 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -287,7 +287,11 @@ static void free_history(struct scrub_file_record **last_scrubs)
 static int cancel_fd = -1;
 static void scrub_sigint_record_progress(int signal)
 {
-	ioctl(cancel_fd, BTRFS_IOC_SCRUB_CANCEL, NULL);
+	int ret;
+
+	ret = ioctl(cancel_fd, BTRFS_IOC_SCRUB_CANCEL, NULL);
+	if (ret < 0)
+		perror("Scrub cancel failed");
 }
 
 static int scrub_handle_sigint_parent(void)

[PATCH 00/14] btrfs-progs: more Coverity cleanups

[Eric Sandeen]

This gets the coverity issue count down to 33.  Before Zach started this
process, we were over 150, IIRC.  So it's almost to the point where the
scans will be manageable going forward.

Not a lot of real bugfixes here, but a bit better error handling in
places.  I sent out 2 dumb patches  (elsewhere) on Friday, though,
so feel free to take a close & skeptical look at these ;)

Thanks,
-Eric

[PATCH 01/14] btrfs-progs: close fd on cmd_subvol_list return

[Eric Sandeen]

stops an fd leak that Coverity found.

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

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 461eed9..a13a58d 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -464,6 +464,8 @@ static int cmd_subvol_list(int argc, char **argv)
 				!is_list_all && !is_only_in_path, NULL);
 
 out:
+	if (fd != -1)
+		close(fd);
 	if (filter_set)
 		btrfs_list_free_filter_set(filter_set);
 	if (comparer_set)
-- 
1.7.1

[PATCH 02/14] btrfs-progs: close fd on do_convert error returns

[Eric Sandeen]

stops an fd leak that Coverity found.

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

diff --git a/convert.c b/convert.c
index 2b3f42f..4a75895 100644
--- a/convert.c
+++ b/convert.c
@@ -2277,7 +2277,8 @@ err:
 
 int do_convert(const char *devname, int datacsum, int packing, int noxattr)
 {
-	int i, fd, ret;
+	int i, ret;
+	int fd = -1;
 	u32 blocksize;
 	u64 blocks[7];
 	u64 total_bytes;
@@ -2407,6 +2408,8 @@ int do_convert(const char *devname, int datacsum, int packing, int noxattr)
 	printf("conversion complete.\n");
 	return 0;
 fail:
+	if (fd != -1)
+		close(fd);
 	fprintf(stderr, "conversion aborted.\n");
 	return -1;
 }
-- 
1.7.1

[PATCH 03/14] btrfs-progs: free resources on do_rollback error returns

[Eric Sandeen]

close fd if open, and free allocated memory in buf

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

diff --git a/convert.c b/convert.c
index 4a75895..76a1076 100644
--- a/convert.c
+++ b/convert.c
@@ -2455,7 +2455,7 @@ fail:
 
 int do_rollback(const char *devname, int force)
 {
-	int fd;
+	int fd = -1;
 	int ret;
 	int i;
 	struct btrfs_root *root;
@@ -2471,7 +2471,7 @@ int do_rollback(const char *devname, int force)
 	struct btrfs_key key;
 	struct btrfs_path path;
 	struct extent_io_tree io_tree;
-	char *buf;
+	char *buf = NULL;
 	char *name;
 	u64 bytenr;
 	u64 num_bytes;
@@ -2751,7 +2751,11 @@ next_sector:
 	extent_io_tree_cleanup(&io_tree);
 	printf("rollback complete.\n");
 	return 0;
+
 fail:
+	if (fd != -1)
+		close(fd);
+	free(buf);
 	fprintf(stderr, "rollback aborted.\n");
 	return -1;
 }
-- 
1.7.1

[PATCH 04/14] btrfs-progs: don't leak fd in get_fs_info

[Eric Sandeen]

If we discover that a passed-in fd is not a mountpoint,
we determine whether it is a device, and issue another
open() against the device's mount point if it is mounted.

If we do so, ensure this 2nd fd gets closed before we return
so that it does not leak, by consolidating error returns.

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

diff --git a/utils.c b/utils.c
index 1813dda..54d577c 100644
--- a/utils.c
+++ b/utils.c
@@ -1462,6 +1462,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		struct btrfs_ioctl_dev_info_args **di_ret)
 {
 	int ret = 0;
+	int fd2 = -1;
 	int ndevs = 0;
 	int i = 1;
 	struct btrfs_fs_devices *fs_devices_mnt = NULL;
@@ -1484,19 +1485,22 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		i = fs_devices_mnt->latest_devid;
 		memcpy(fi_args->fsid, fs_devices_mnt->fsid, BTRFS_FSID_SIZE);
 		close(fd);
-		fd = open_file_or_dir(mp);
-		if (fd < 0)
+		fd2 = open_file_or_dir(mp);
+		if (fd2 < 0)
 			return -errno;
+		fd = fd2;
 	} else if (ret) {
 		return -errno;
 	}
 
 	if (!fi_args->num_devices)
-		return 0;
+		goto out;
 
 	di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
-	if (!di_args)
-		return -errno;
+	if (!di_args) {
+		ret = -errno;
+		goto out;
+	}
 
 	for (; i <= fi_args->max_id; ++i) {
 		BUG_ON(ndevs >= fi_args->num_devices);
@@ -1504,13 +1508,16 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		if (ret == -ENODEV)
 			continue;
 		if (ret)
-			return ret;
+			goto out;
 		ndevs++;
 	}
 
 	BUG_ON(ndevs == 0);
 
-	return 0;
+out:
+	if (fd2 != -1)
+		close(fd2);
+	return ret;
 }
 
 #define isoctal(c)	(((c) & ~7) == '0')
-- 
1.7.1

[PATCH 05/14] btrfs-progs: free allocated metadump structure on restore failure

[Eric Sandeen]

Don't return w/ "metadump" still allocated

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

diff --git a/btrfs-image.c b/btrfs-image.c
index a54e6c9..5b0af28 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -765,11 +765,11 @@ static int wait_for_worker(struct mdrestore_struct *mdres)
 
 static int restore_metadump(const char *input, FILE *out, int num_threads)
 {
-	struct meta_cluster *cluster;
+	struct meta_cluster *cluster = NULL;
 	struct meta_cluster_header *header;
 	struct mdrestore_struct mdrestore;
 	u64 bytenr = 0;
-	FILE *in;
+	FILE *in = NULL;
 	int ret;
 
 	if (!strcmp(input, "-")) {
@@ -797,14 +797,15 @@ static int restore_metadump(const char *input, FILE *out, int num_threads)
 		if (le64_to_cpu(header->magic) != HEADER_MAGIC ||
 		    le64_to_cpu(header->bytenr) != bytenr) {
 			fprintf(stderr, "bad header in metadump image\n");
-			return 1;
+			ret = 1;
+			goto out;
 		}
 		ret = add_cluster(cluster, &mdrestore, &bytenr);
 		BUG_ON(ret);
 
 		wait_for_worker(&mdrestore);
 	}
-
+out:
 	mdrestore_destroy(&mdrestore);
 	free(cluster);
 	if (in != stdin)
-- 
1.7.1

[PATCH 06/14] btrfs-progs: check for null string in parse_size

[Eric Sandeen]

Because it's better than a segfault if it's called improperly,
and it makes static checkers happier.

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

diff --git a/utils.c b/utils.c
index 54d577c..a38ac70 100644
--- a/utils.c
+++ b/utils.c
@@ -1382,7 +1382,7 @@ u64 parse_size(char *s)
 	char c;
 	u64 mult = 1;
 
-	for (i=0 ; s[i] && isdigit(s[i]) ; i++) ;
+	for (i=0 ; s && s[i] && isdigit(s[i]) ; i++) ;
 	if (!i) {
 		fprintf(stderr, "ERROR: size value is empty\n");
 		exit(50);
-- 
1.7.1

[PATCH 07/14] btrfs-progs: tidy up cmd_snapshot() whitespace & returns

[Eric Sandeen]

Just whitespace fixes, and magical return value removal.

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

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index a13a58d..a4d88a1 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -533,57 +533,57 @@ static int cmd_snapshot(int argc, char **argv)
 	dst = argv[optind + 1];
 
 	res = test_issubvolume(subvol);
-	if(res<0){
+	if (res < 0) {
 		fprintf(stderr, "ERROR: error accessing '%s'\n", subvol);
-		return 12;
+		return 1;
 	}
-	if(!res){
+	if (!res) {
 		fprintf(stderr, "ERROR: '%s' is not a subvolume\n", subvol);
-		return 13;
+		return 1;
 	}
 
 	res = test_isdir(dst);
-	if(res == 0 ){
+	if (res == 0) {
 		fprintf(stderr, "ERROR: '%s' exists and it is not a directory\n", dst);
-		return 12;
+		return 1;
 	}
 
-	if(res>0){
+	if (res > 0) {
 		newname = strdup(subvol);
 		newname = basename(newname);
 		dstdir = dst;
-	}else{
+	} else {
 		newname = strdup(dst);
 		newname = basename(newname);
 		dstdir = strdup(dst);
 		dstdir = dirname(dstdir);
 	}
 
-	if( !strcmp(newname,".") || !strcmp(newname,"..") ||
+	if (!strcmp(newname,".") || !strcmp(newname,"..") ||
 	     strchr(newname, '/') ){
 		fprintf(stderr, "ERROR: incorrect snapshot name ('%s')\n",
 			newname);
-		return 14;
+		return 1;
 	}
 
 	len = strlen(newname);
 	if (len == 0 || len >= BTRFS_VOL_NAME_MAX) {
 		fprintf(stderr, "ERROR: snapshot name too long ('%s)\n",
 			newname);
-		return 14;
+		return 1;
 	}
 
 	fddst = open_file_or_dir(dstdir);
 	if (fddst < 0) {
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
-		return 12;
+		return 1;
 	}
 
 	fd = open_file_or_dir(subvol);
 	if (fd < 0) {
 		close(fddst);
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
-		return 12;
+		return 1;
 	}
 
 	if (readonly) {
@@ -609,10 +609,10 @@ static int cmd_snapshot(int argc, char **argv)
 	close(fddst);
 	free(inherit);
 
-	if(res < 0 ){
+	if (res < 0) {
 		fprintf( stderr, "ERROR: cannot snapshot '%s' - %s\n",
 			subvol, strerror(e));
-		return 11;
+		return 1;
 	}
 
 	return 0;
-- 
1.7.1

[PATCH 08/14] btrfs-progs: Free resources when returning error from cmd_snapshot()

[Eric Sandeen]

cmd_snapshot() currently returns without freeing resources
in almost every error case.  Switch to a goto arrangement
so all cleanup can be done in one place.

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

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index a4d88a1..96f7cbd 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -489,7 +489,9 @@ static const char * const cmd_snapshot_usage[] = {
 static int cmd_snapshot(int argc, char **argv)
 {
 	char	*subvol, *dst;
-	int	res, fd, fddst, len, e, readonly = 0;
+	int	res, retval;
+	int	fd = -1, fddst = -1;
+	int	len, readonly = 0;
 	char	*newname;
 	char	*dstdir;
 	struct btrfs_ioctl_vol_args_v2	args;
@@ -532,20 +534,21 @@ static int cmd_snapshot(int argc, char **argv)
 	subvol = argv[optind];
 	dst = argv[optind + 1];
 
+	retval = 1;	/* failure */
 	res = test_issubvolume(subvol);
 	if (res < 0) {
 		fprintf(stderr, "ERROR: error accessing '%s'\n", subvol);
-		return 1;
+		goto out;
 	}
 	if (!res) {
 		fprintf(stderr, "ERROR: '%s' is not a subvolume\n", subvol);
-		return 1;
+		goto out;
 	}
 
 	res = test_isdir(dst);
 	if (res == 0) {
 		fprintf(stderr, "ERROR: '%s' exists and it is not a directory\n", dst);
-		return 1;
+		goto out;
 	}
 
 	if (res > 0) {
@@ -563,27 +566,26 @@ static int cmd_snapshot(int argc, char **argv)
 	     strchr(newname, '/') ){
 		fprintf(stderr, "ERROR: incorrect snapshot name ('%s')\n",
 			newname);
-		return 1;
+		goto out;
 	}
 
 	len = strlen(newname);
 	if (len == 0 || len >= BTRFS_VOL_NAME_MAX) {
 		fprintf(stderr, "ERROR: snapshot name too long ('%s)\n",
 			newname);
-		return 1;
+		goto out;
 	}
 
 	fddst = open_file_or_dir(dstdir);
 	if (fddst < 0) {
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
-		return 1;
+		goto out;
 	}
 
 	fd = open_file_or_dir(subvol);
 	if (fd < 0) {
-		close(fddst);
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
-		return 1;
+		goto out;
 	}
 
 	if (readonly) {
@@ -602,20 +604,25 @@ static int cmd_snapshot(int argc, char **argv)
 		args.qgroup_inherit = inherit;
 	}
 	strncpy_null(args.name, newname);
-	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
-	e = errno;
 
-	close(fd);
-	close(fddst);
-	free(inherit);
+	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 
 	if (res < 0) {
 		fprintf( stderr, "ERROR: cannot snapshot '%s' - %s\n",
-			subvol, strerror(e));
-		return 1;
+			subvol, strerror(errno));
+		goto out;
 	}
 
-	return 0;
+	retval = 0;	/* success */
+
+out:
+	if (fd != -1)
+		close(fd);
+	if (fddst != -1)
+		close(fddst);
+	free(inherit);
+
+	return retval;
 }
 
 static const char * const cmd_subvol_get_default_usage[] = {
-- 
1.7.1

[PATCH 09/14] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns

[Eric Sandeen]

Just whitespace fixes, and magical return value removal.

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

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 96f7cbd..b7777ee 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -104,9 +104,9 @@ static int cmd_subvol_create(int argc, char **argv)
 	dst = argv[optind];
 
 	res = test_isdir(dst);
-	if(res >= 0 ){
+	if (res >= 0) {
 		fprintf(stderr, "ERROR: '%s' exists\n", dst);
-		return 12;
+		return 1;
 	}
 
 	newname = strdup(dst);
@@ -114,24 +114,24 @@ static int cmd_subvol_create(int argc, char **argv)
 	dstdir = strdup(dst);
 	dstdir = dirname(dstdir);
 
-	if( !strcmp(newname,".") || !strcmp(newname,"..") ||
+	if (!strcmp(newname,".") || !strcmp(newname,"..") ||
 	     strchr(newname, '/') ){
 		fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n",
 			newname);
-		return 14;
+		return 1;
 	}
 
 	len = strlen(newname);
 	if (len == 0 || len >= BTRFS_VOL_NAME_MAX) {
 		fprintf(stderr, "ERROR: subvolume name too long ('%s)\n",
 			newname);
-		return 14;
+		return 1;
 	}
 
 	fddst = open_file_or_dir(dstdir);
 	if (fddst < 0) {
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
-		return 12;
+		return 1;
 	}
 
 	printf("Create subvolume '%s/%s'\n", dstdir, newname);
@@ -159,10 +159,10 @@ static int cmd_subvol_create(int argc, char **argv)
 	close(fddst);
 	free(inherit);
 
-	if(res < 0 ){
-		fprintf( stderr, "ERROR: cannot create subvolume - %s\n",
+	if (res < 0) {
+		fprintf(stderr, "ERROR: cannot create subvolume - %s\n",
 			strerror(e));
-		return 11;
+		return 1;
 	}
 
 	return 0;
-- 
1.7.1

[PATCH 10/14] btrfs-progs: Free resources when returning error from cmd_subvol_create()

[Eric Sandeen]

cmd_subvol_create() currently returns without freeing resources
in almost every error case.  Switch to a goto arrangement
so all cleanup can be done in one place.

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

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index b7777ee..bfea0d9 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -70,7 +70,8 @@ static const char * const cmd_subvol_create_usage[] = {
 
 static int cmd_subvol_create(int argc, char **argv)
 {
-	int	res, fddst, len, e;
+	int	retval, res, len;
+	int	fddst = -1;
 	char	*newname;
 	char	*dstdir;
 	char	*dst;
@@ -103,10 +104,11 @@ static int cmd_subvol_create(int argc, char **argv)
 
 	dst = argv[optind];
 
+	retval = 1;	/* failure */
 	res = test_isdir(dst);
 	if (res >= 0) {
 		fprintf(stderr, "ERROR: '%s' exists\n", dst);
-		return 1;
+		goto out;
 	}
 
 	newname = strdup(dst);
@@ -118,20 +120,20 @@ static int cmd_subvol_create(int argc, char **argv)
 	     strchr(newname, '/') ){
 		fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n",
 			newname);
-		return 1;
+		goto out;
 	}
 
 	len = strlen(newname);
 	if (len == 0 || len >= BTRFS_VOL_NAME_MAX) {
 		fprintf(stderr, "ERROR: subvolume name too long ('%s)\n",
 			newname);
-		return 1;
+		goto out;
 	}
 
 	fddst = open_file_or_dir(dstdir);
 	if (fddst < 0) {
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
-		return 1;
+		goto out;
 	}
 
 	printf("Create subvolume '%s/%s'\n", dstdir, newname);
@@ -154,18 +156,19 @@ static int cmd_subvol_create(int argc, char **argv)
 		res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
 	}
 
-	e = errno;
-
-	close(fddst);
-	free(inherit);
-
 	if (res < 0) {
 		fprintf(stderr, "ERROR: cannot create subvolume - %s\n",
-			strerror(e));
-		return 1;
+			strerror(errno));
+		goto out;
 	}
 
-	return 0;
+	retval = 0;	/* success */
+out:
+	if (fddst != -1)
+		close(fddst);
+	free(inherit);
+
+	return retval;
 }
 
 /*
-- 
1.7.1

[PATCH 11/14] btrfs-progs: check return of posix_fadvise

[Eric Sandeen]

It seems highly unlikely that posix_fadvise could fail,
and even if it does, it was only advisory.  Still, if
it does, we could issue a notice to the user.

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

diff --git a/disk-io.c b/disk-io.c
index 897d0cf..97fbfbd 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -822,7 +822,8 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 		sb_bytenr = BTRFS_SUPER_INFO_OFFSET;
 
 	/* try to drop all the caches */
-	posix_fadvise(fp, 0, 0, POSIX_FADV_DONTNEED);
+	if (posix_fadvise(fp, 0, 0, POSIX_FADV_DONTNEED))
+		fprintf(stderr, "Warning, could not drop caches\n");
 
 	ret = btrfs_scan_one_device(fp, path, &fs_devices,
 				    &total_devs, sb_bytenr);
@@ -1282,7 +1283,8 @@ static int close_all_devices(struct btrfs_fs_info *fs_info)
 		device = list_entry(next, struct btrfs_device, dev_list);
 		if (device->fd) {
 			fsync(device->fd);
-			posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED);
+			if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED))
+				fprintf(stderr, "Warning, could not drop caches\n");
 		}
 		close(device->fd);
 	}
diff --git a/volumes.c b/volumes.c
index ca1b402..c0d02d1 100644
--- a/volumes.c
+++ b/volumes.c
@@ -193,7 +193,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, int flags)
 			goto fail;
 		}
 
-		posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);
+		if (posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED))
+			fprintf(stderr, "Warning, could not drop caches\n");
 
 		if (device->devid == fs_devices->latest_devid)
 			fs_devices->latest_bdev = fd;
-- 
1.7.1

[PATCH 12/14] btrfs-progs: Issue warnings if ioctls fail in sigint handlers

[Eric Sandeen]

The two sigint handlers issue ioctls to clean up, but if
they fail, noone would know.  I'm not sure there is
any other error handling to be done at this point, but a
notification seems wise.

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

diff --git a/cmds-replace.c b/cmds-replace.c
index 4cc32df..10030f6 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -77,10 +77,13 @@ static int is_numerical(const char *str)
 static int dev_replace_cancel_fd = -1;
 static void dev_replace_sigint_handler(int signal)
 {
+	int ret;
 	struct btrfs_ioctl_dev_replace_args args = {0};
 
 	args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_CANCEL;
-	ioctl(dev_replace_cancel_fd, BTRFS_IOC_DEV_REPLACE, &args);
+	ret = ioctl(dev_replace_cancel_fd, BTRFS_IOC_DEV_REPLACE, &args);
+	if (ret < 0)
+		perror("Device replace cancel failed");
 }
 
 static int dev_replace_handle_sigint(int fd)
diff --git a/cmds-scrub.c b/cmds-scrub.c
index da4120f..f73b3c6 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -287,7 +287,11 @@ static void free_history(struct scrub_file_record **last_scrubs)
 static int cancel_fd = -1;
 static void scrub_sigint_record_progress(int signal)
 {
-	ioctl(cancel_fd, BTRFS_IOC_SCRUB_CANCEL, NULL);
+	int ret;
+
+	ret = ioctl(cancel_fd, BTRFS_IOC_SCRUB_CANCEL, NULL);
+	if (ret < 0)
+		perror("Scrub cancel failed\n");
 }
 
 static int scrub_handle_sigint_parent(void)
-- 
1.7.1

[PATCH 13/14] btrfs-progs: better option/error handling for btrfs-vol

[Eric Sandeen]

Today wrong cmdlines give odd results:

	# ./btrfs-vol /dev/sdb1
	Unable to open device (null)
	# ./btrfs-vol -a /dev/sdb1
	usage: btrfs-vol [options] mount_point ...

Make it a bit more informative:

	# ./btrfs-vol /dev/sdb1
	No command specified
	usage: btrfs-vol [options] mount_point ...
	# ./btrfs-vol -a /dev/sdb1
	No mountpoint specified
	usage: btrfs-vol [options] mount_point ...

(even though it's deprecated ...)

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

diff --git a/btrfs-vol.c b/btrfs-vol.c
index 3cc1c32..e37b41a 100644
--- a/btrfs-vol.c
+++ b/btrfs-vol.c
@@ -106,8 +106,13 @@ int main(int ac, char **av)
 		}
 	}
 	ac = ac - optind;
-	if (ac == 0)
+	if (ac == 0 || !cmd) {
+		if (!ac)
+			fprintf(stderr, "No mountpoint specified\n");
+		else
+			fprintf(stderr, "No command specified\n");
 		print_usage();
+	}
 	mnt = av[optind];
 
 	if (device && strcmp(device, "missing") == 0 &&
-- 
1.7.1

[PATCH 14/14] btrfs-progs: Error handling in scrub_progress_cycle() thread

[Eric Sandeen]

consolidate error handling to ensure that peer_fd
is closed on error paths.  Add a couple comments
to the error handling after the thread is complete.

Note that scrub_progress_cycle returns negative
errnos on any error.

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

diff --git a/cmds-scrub.c b/cmds-scrub.c
index f73b3c6..b0d4717 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -840,6 +840,7 @@ static void *progress_one_dev(void *ctx)
 	return NULL;
 }
 
+/* nb: returns a negative errno via ERR_PTR */
 static void *scrub_progress_cycle(void *ctx)
 {
 	int ret;
@@ -867,9 +868,9 @@ static void *scrub_progress_cycle(void *ctx)
 	struct sockaddr_un peer;
 	socklen_t peer_size = sizeof(peer);
 
-	ret = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
+	ret = -pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
 	if (ret)
-		return ERR_PTR(-ret);
+		goto out;
 
 	uuid_unparse(spc->fi->fsid, fsid);
 
@@ -890,8 +891,10 @@ static void *scrub_progress_cycle(void *ctx)
 
 	while (1) {
 		ret = poll(&accept_poll_fd, 1, 5 * 1000);
-		if (ret == -1)
-			return ERR_PTR(-errno);
+		if (ret == -1) {
+			ret = -errno;
+			goto out;
+		}
 		if (ret)
 			peer_fd = accept(spc->prg_fd, (struct sockaddr *)&peer,
 					 &peer_size);
@@ -909,42 +912,46 @@ static void *scrub_progress_cycle(void *ctx)
 			if (!sp->ret)
 				continue;
 			if (sp->ioctl_errno != ENOTCONN &&
-			    sp->ioctl_errno != ENODEV)
-				return ERR_PTR(-sp->ioctl_errno);
+			    sp->ioctl_errno != ENODEV) {
+				ret = -sp->ioctl_errno;
+				goto out;
+			}
 			/*
 			 * scrub finished or device removed, check the
 			 * finished flag. if unset, just use the last
 			 * result we got for the current write and go
 			 * on. flag should be set on next cycle, then.
 			 */
-			ret = pthread_mutex_lock(&sp_shared->progress_mutex);
+			ret = -pthread_mutex_lock(&sp_shared->progress_mutex);
 			if (ret)
-				return ERR_PTR(-ret);
+				goto out;
 			if (!sp_shared->stats.finished) {
-				ret = pthread_mutex_unlock(
+				ret = -pthread_mutex_unlock(
 						&sp_shared->progress_mutex);
 				if (ret)
-					return ERR_PTR(-ret);
+					goto out;
 				memcpy(sp, sp_last, sizeof(*sp));
 				continue;
 			}
-			ret = pthread_mutex_unlock(&sp_shared->progress_mutex);
+			ret = -pthread_mutex_unlock(&sp_shared->progress_mutex);
 			if (ret)
-				return ERR_PTR(-ret);
+				goto out;
 			memcpy(sp, sp_shared, sizeof(*sp));
 			memcpy(sp_last, sp_shared, sizeof(*sp));
 		}
 		if (peer_fd != -1) {
 			write_poll_fd.fd = peer_fd;
 			ret = poll(&write_poll_fd, 1, 0);
-			if (ret == -1)
-				return ERR_PTR(-errno);
+			if (ret == -1) {
+				ret = -errno;
+				goto out;
+			}
 			if (ret) {
 				ret = scrub_write_file(
 					peer_fd, fsid,
 					&spc->progress[this * ndev], ndev);
 				if (ret)
-					return ERR_PTR(ret);
+					goto out;
 			}
 			close(peer_fd);
 			peer_fd = -1;
@@ -954,8 +961,12 @@ static void *scrub_progress_cycle(void *ctx)
 		ret = scrub_write_progress(spc->write_mutex, fsid,
 					   &spc->progress[this * ndev], ndev);
 		if (ret)
-			return ERR_PTR(ret);
+			goto out;
 	}
+out:
+	if (peer_fd != -1)
+		close(peer_fd);
+	return ERR_PTR(ret);
 }
 
 static struct scrub_file_record *last_dev_scrub(
@@ -1373,11 +1384,14 @@ static int scrub_start(int argc, char **argv, int resume)
 	ret = pthread_cancel(t_prog);
 	if (!ret)
 		ret = pthread_join(t_prog, &terr);
+
+	/* check for errors from the handling of the progress thread */
 	if (do_print && ret) {
-		fprintf(stderr, "ERROR: progress thead handling failed: %s\n",
+		fprintf(stderr, "ERROR: progress thread handling failed: %s\n",
 			strerror(ret));
 	}
 
+	/* check for errors returned from the progress thread itself */
 	if (do_print && terr && terr != PTHREAD_CANCELED) {
 		fprintf(stderr, "ERROR: recording progress "
 			"failed: %s\n", strerror(-PTR_ERR(terr)));
-- 
1.7.1

[PATCH 14/14 V2] btrfs-progs: Error handling in scrub_progress_cycle() thread

[Eric Sandeen]

consolidate error handling to ensure that peer_fd
is closed on error paths.  Add a couple comments
to the error handling after the thread is complete.

Note that scrub_progress_cycle returns negative
errnos on any error.

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

V2: collect positive / pthread errors in perr & flip
on return if needed

diff --git a/cmds-scrub.c b/cmds-scrub.c
index f73b3c6..748a329 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -840,9 +840,11 @@ static void *progress_one_dev(void *ctx)
 	return NULL;
 }
 
+/* nb: returns a negative errno via ERR_PTR */
 static void *scrub_progress_cycle(void *ctx)
 {
 	int ret;
+	int  perr = 0;	/* positive / pthread error returns */
 	int old;
 	int i;
 	char fsid[37];
@@ -867,9 +869,9 @@ static void *scrub_progress_cycle(void *ctx)
 	struct sockaddr_un peer;
 	socklen_t peer_size = sizeof(peer);
 
-	ret = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
-	if (ret)
-		return ERR_PTR(-ret);
+	perr = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
+	if (perr)
+		goto out;
 
 	uuid_unparse(spc->fi->fsid, fsid);
 
@@ -890,8 +892,10 @@ static void *scrub_progress_cycle(void *ctx)
 
 	while (1) {
 		ret = poll(&accept_poll_fd, 1, 5 * 1000);
-		if (ret == -1)
-			return ERR_PTR(-errno);
+		if (ret == -1) {
+			ret = -errno;
+			goto out;
+		}
 		if (ret)
 			peer_fd = accept(spc->prg_fd, (struct sockaddr *)&peer,
 					 &peer_size);
@@ -909,42 +913,46 @@ static void *scrub_progress_cycle(void *ctx)
 			if (!sp->ret)
 				continue;
 			if (sp->ioctl_errno != ENOTCONN &&
-			    sp->ioctl_errno != ENODEV)
-				return ERR_PTR(-sp->ioctl_errno);
+			    sp->ioctl_errno != ENODEV) {
+				ret = -sp->ioctl_errno;
+				goto out;
+			}
 			/*
 			 * scrub finished or device removed, check the
 			 * finished flag. if unset, just use the last
 			 * result we got for the current write and go
 			 * on. flag should be set on next cycle, then.
 			 */
-			ret = pthread_mutex_lock(&sp_shared->progress_mutex);
-			if (ret)
-				return ERR_PTR(-ret);
+			perr = pthread_mutex_lock(&sp_shared->progress_mutex);
+			if (perr)
+				goto out;
 			if (!sp_shared->stats.finished) {
-				ret = pthread_mutex_unlock(
+				perr = pthread_mutex_unlock(
 						&sp_shared->progress_mutex);
-				if (ret)
-					return ERR_PTR(-ret);
+				if (perr)
+					goto out;
 				memcpy(sp, sp_last, sizeof(*sp));
 				continue;
 			}
-			ret = pthread_mutex_unlock(&sp_shared->progress_mutex);
-			if (ret)
-				return ERR_PTR(-ret);
+			perr = pthread_mutex_unlock(&sp_shared->progress_mutex);
+			if (perr)
+				goto out;
 			memcpy(sp, sp_shared, sizeof(*sp));
 			memcpy(sp_last, sp_shared, sizeof(*sp));
 		}
 		if (peer_fd != -1) {
 			write_poll_fd.fd = peer_fd;
 			ret = poll(&write_poll_fd, 1, 0);
-			if (ret == -1)
-				return ERR_PTR(-errno);
+			if (ret == -1) {
+				ret = -errno;
+				goto out;
+			}
 			if (ret) {
 				ret = scrub_write_file(
 					peer_fd, fsid,
 					&spc->progress[this * ndev], ndev);
 				if (ret)
-					return ERR_PTR(ret);
+					goto out;
 			}
 			close(peer_fd);
 			peer_fd = -1;
@@ -954,8 +962,14 @@ static void *scrub_progress_cycle(void *ctx)
 		ret = scrub_write_progress(spc->write_mutex, fsid,
 					   &spc->progress[this * ndev], ndev);
 		if (ret)
-			return ERR_PTR(ret);
+			goto out;
 	}
+out:
+	if (peer_fd != -1)
+		close(peer_fd);
+	if (perr)
+		ret = -perr;
+	return ERR_PTR(ret);
 }
 
 static struct scrub_file_record *last_dev_scrub(
@@ -1373,11 +1387,14 @@ static int scrub_start(int argc, char **argv, int resume)
 	ret = pthread_cancel(t_prog);
 	if (!ret)
 		ret = pthread_join(t_prog, &terr);
+
+	/* check for errors from the handling of the progress thread */
 	if (do_print && ret) {
-		fprintf(stderr, "ERROR: progress thead handling failed: %s\n",
+		fprintf(stderr, "ERROR: progress thread handling failed: %s\n",
 			strerror(ret));
 	}
 
+	/* check for errors returned from the progress thread itself */
 	if (do_print && terr && terr != PTHREAD_CANCELED) {
 		fprintf(stderr, "ERROR: recording progress "
 			"failed: %s\n", strerror(-PTR_ERR(terr)));

[PATCH 15/14] btrfs-progs: fix scrub error return from pthread_mutex_lock

[Eric Sandeen]

If pthread_mutex_lock() fails it returns the error in ret,
and does not set errno.

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

diff --git a/cmds-scrub.c b/cmds-scrub.c
index f73b3c6..8129601 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -763,7 +763,7 @@ static int scrub_write_progress(pthread_mutex_t *m, const char *fsid,
 
 	ret = pthread_mutex_lock(m);
 	if (ret) {
-		err = -errno;
+		err = -ret;
 		goto out;
 	}
 

Re: [PATCH 14/14 V2] btrfs-progs: Error handling in scrub_progress_cycle() thread

[Zach Brown]

> +	perr = pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, &old);
> +	if (perr)
> +		goto out;
>  

> +out:
> +	if (peer_fd != -1)
> +		close(peer_fd);
> +	if (perr)
> +		ret = -perr;
> +	return ERR_PTR(ret);
>  }

Great, that's a lot clearer.  Thanks.

- z

Re: [PATCH 04/14] btrfs-progs: don't leak fd in get_fs_info

[Eric Sandeen]

On 3/4/13 4:39 PM, Eric Sandeen wrote:
> If we discover that a passed-in fd is not a mountpoint,
> we determine whether it is a device, and issue another
> open() against the device's mount point if it is mounted.
> 
> If we do so, ensure this 2nd fd gets closed before we return
> so that it does not leak, by consolidating error returns.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Gah, self-nak on this for now, I started trying to make a
regression test for scrub, and this makes it fail.

Don't know why yet.

-Eric

> ---
>  utils.c |   21 ++++++++++++++-------
>  1 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/utils.c b/utils.c
> index 1813dda..54d577c 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1462,6 +1462,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>  		struct btrfs_ioctl_dev_info_args **di_ret)
>  {
>  	int ret = 0;
> +	int fd2 = -1;
>  	int ndevs = 0;
>  	int i = 1;
>  	struct btrfs_fs_devices *fs_devices_mnt = NULL;
> @@ -1484,19 +1485,22 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>  		i = fs_devices_mnt->latest_devid;
>  		memcpy(fi_args->fsid, fs_devices_mnt->fsid, BTRFS_FSID_SIZE);
>  		close(fd);
> -		fd = open_file_or_dir(mp);
> -		if (fd < 0)
> +		fd2 = open_file_or_dir(mp);
> +		if (fd2 < 0)
>  			return -errno;
> +		fd = fd2;
>  	} else if (ret) {
>  		return -errno;
>  	}
>  
>  	if (!fi_args->num_devices)
> -		return 0;
> +		goto out;
>  
>  	di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
> -	if (!di_args)
> -		return -errno;
> +	if (!di_args) {
> +		ret = -errno;
> +		goto out;
> +	}
>  
>  	for (; i <= fi_args->max_id; ++i) {
>  		BUG_ON(ndevs >= fi_args->num_devices);
> @@ -1504,13 +1508,16 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>  		if (ret == -ENODEV)
>  			continue;
>  		if (ret)
> -			return ret;
> +			goto out;
>  		ndevs++;
>  	}
>  
>  	BUG_ON(ndevs == 0);
>  
> -	return 0;
> +out:
> +	if (fd2 != -1)
> +		close(fd2);
> +	return ret;
>  }
>  
>  #define isoctal(c)	(((c) & ~7) == '0')
> 

Re: [PATCH 04/14] btrfs-progs: don't leak fd in get_fs_info

[Eric Sandeen]

On 3/5/13 5:41 PM, Eric Sandeen wrote:
> On 3/4/13 4:39 PM, Eric Sandeen wrote:
>> If we discover that a passed-in fd is not a mountpoint,
>> we determine whether it is a device, and issue another
>> open() against the device's mount point if it is mounted.
>>
>> If we do so, ensure this 2nd fd gets closed before we return
>> so that it does not leak, by consolidating error returns.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Gah, self-nak on this for now, I started trying to make a
> regression test for scrub, and this makes it fail.
> 
> Don't know why yet.
> 
> -Eric

For posterity, it's because this function is actually
doing kind of a nasty thing - it closes the caller's
filehandle & re-opens it on a different path.

Usually the caller is none the wiser, but ick!  So permanent
NAK on this patch, I'm working on a different solution.

-Eric

Re: [PATCH 00/14] btrfs-progs: more Coverity cleanups

[David Sterba]

On Mon, Mar 04, 2013 at 04:39:50PM -0600, Eric Sandeen wrote:
> This gets the coverity issue count down to 33.  Before Zach started this
> process, we were over 150, IIRC.  So it's almost to the point where the
> scans will be manageable going forward.
> 
> Not a lot of real bugfixes here, but a bit better error handling in
> places.  I sent out 2 dumb patches  (elsewhere) on Friday, though,
> so feel free to take a close & skeptical look at these ;)

Thanks, pulled them in (minus 04 and V2 for 12 and 14).

david