thr3ads.net - Btrfs devel - [PATCH 00/14] btrfs-progs: more Coverity cleanups [Mar 2013]

If this information is useful, please help other people find it:
Share via:

Zach Brown

2013-Mar-04 21:57 UTC

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

> +	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
--
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

Zach Brown

2013-Mar-04 22:00 UTC

head link

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

> -	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
--
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

Zach Brown

2013-Mar-04 22:02 UTC

head link

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

> 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
--
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

Eric Sandeen

2013-Mar-04 22:34 UTC

head link

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

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
--
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

Eric Sandeen

2013-Mar-04 22:34 UTC

head link

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

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.
--
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

Eric Sandeen

2013-Mar-04 22:35 UTC

head link

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

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)

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:39 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:40 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:40 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:40 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:40 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:40 UTC

head link

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

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

--
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

Eric Sandeen

2013-Mar-04 22:45 UTC

head link

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

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)));

--
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

Eric Sandeen

2013-Mar-04 22:49 UTC

head link

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

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;
 	}
 
--
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

Zach Brown

2013-Mar-04 23:12 UTC

head link

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

> +	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
--
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

Eric Sandeen

2013-Mar-05 23:41 UTC

head link

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

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'')
> 
--
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

Eric Sandeen

2013-Mar-08 15:27 UTC

head link

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

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
--
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

David Sterba

2013-Mar-10 15:19 UTC

head link

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

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
--
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

Btrfs devel - Mar 2013 - [PATCH 00/14] btrfs-progs: more Coverity cleanups

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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