[PATCH 1/2] btrfs-progs: don't report internal dev replace result if ioctl failed

[PATCH 1/2] btrfs-progs: don't report internal dev replace result if ioctl failed

[Eryu Guan]

If BTRFS_IOC_DEV_REPLACE ioctl failed, there's no result returned to
fill args.result, it doesn't make sense to report this internal result
to user.

And the arg has been initialized with 0, the result is always 0, which
is BTRFS_IOCTL_DEV_REPLACE_REPLACE_NO_ERROR, and the resulting error
message looks confusing too:

ERROR: ioctl(DEV_REPLACE_START) failed on "/mnt/btrfs": No such file or directory, no error

So just skip the internal dev replace result if the whole ioctl failed.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 cmds-replace.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index 9fe7ad8..51b7aaf 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -187,9 +187,8 @@ static int cmd_start_replace(int argc, char **argv)
 	ret = ioctl(fdmnt, BTRFS_IOC_DEV_REPLACE, &status_args);
 	if (ret) {
 		fprintf(stderr,
-			"ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s, %s\n",
-			path, strerror(errno),
-			replace_dev_result2string(status_args.result));
+			"ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s\n",
+			path, strerror(errno));
 		goto leave_with_error;
 	}
 
@@ -305,9 +304,8 @@ static int cmd_start_replace(int argc, char **argv)
 	if (do_not_background) {
 		if (ret) {
 			fprintf(stderr,
-				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s, %s\n",
-				path, strerror(errno),
-				replace_dev_result2string(start_args.result));
+				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s\n",
+				path, strerror(errno));
 
 			if (errno == EOPNOTSUPP)
 				fprintf(stderr,
@@ -404,9 +402,8 @@ static int print_replace_status(int fd, const char *path, int once)
 		args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS;
 		ret = ioctl(fd, BTRFS_IOC_DEV_REPLACE, &args);
 		if (ret) {
-			fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s, %s\n",
-				path, strerror(errno),
-				replace_dev_result2string(args.result));
+			fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s\n",
+				path, strerror(errno));
 			return ret;
 		}
 
@@ -554,9 +551,8 @@ static int cmd_cancel_replace(int argc, char **argv)
 	e = errno;
 	close_file_or_dir(fd, dirstream);
 	if (ret) {
-		fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_CANCEL) failed on \"%s\": %s, %s\n",
-			path, strerror(e),
-			replace_dev_result2string(args.result));
+		fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_CANCEL) failed on \"%s\": %s\n",
+			path, strerror(e));
 		return 1;
 	}
 	if (args.result == BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED) {
-- 
1.8.3.1

[PATCH 2/2] btrfs-progs: return error when canonicalize_path failed

[Eryu Guan]

Error out cmd_start_replace() if canonicalize_path() dstdev failed, add
the missing "goto leave_with_error;"

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 cmds-replace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cmds-replace.c b/cmds-replace.c
index 51b7aaf..d5648d5 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -213,6 +213,7 @@ static int cmd_start_replace(int argc, char **argv)
 		fprintf(stderr,
 			"ERROR: Could not canonicalize path '%s': %s\n",
 			argv[optind + 1], strerror(errno));
+		goto leave_with_error;
 	}
 
 	if (is_numerical(srcdev)) {
-- 
1.8.3.1

Re: [PATCH 1/2] btrfs-progs: don't report internal dev replace result if ioctl failed

[David Sterba]

On Wed, Oct 08, 2014 at 05:42:28PM +0800, Eryu Guan wrote:
> If BTRFS_IOC_DEV_REPLACE ioctl failed, there's no result returned to
> fill args.result, it doesn't make sense to report this internal result
> to user.
> 
> And the arg has been initialized with 0, the result is always 0, which
> is BTRFS_IOCTL_DEV_REPLACE_REPLACE_NO_ERROR, and the resulting error
> message looks confusing too:
> 
> ERROR: ioctl(DEV_REPLACE_START) failed on "/mnt/btrfs": No such file or directory, no error
> 
> So just skip the internal dev replace result if the whole ioctl failed.

The 'no error' is confusing there, but I'm afraid we're losing some
information if the secondary result is completely dropped. How about
intializing the replace result with, eg., -1 and then print an empty
string from replace_dev_result2string instead?

Re: [PATCH 1/2] btrfs-progs: don't report internal dev replace result if ioctl failed

[Eryu Guan]

On Fri, Oct 10, 2014 at 10:20:23AM +0200, David Sterba wrote:
> On Wed, Oct 08, 2014 at 05:42:28PM +0800, Eryu Guan wrote:
> > If BTRFS_IOC_DEV_REPLACE ioctl failed, there's no result returned to
> > fill args.result, it doesn't make sense to report this internal result
> > to user.
> > 
> > And the arg has been initialized with 0, the result is always 0, which
> > is BTRFS_IOCTL_DEV_REPLACE_REPLACE_NO_ERROR, and the resulting error
> > message looks confusing too:
> > 
> > ERROR: ioctl(DEV_REPLACE_START) failed on "/mnt/btrfs": No such file or directory, no error
> > 
> > So just skip the internal dev replace result if the whole ioctl failed.
> 
> The 'no error' is confusing there, but I'm afraid we're losing some
> information if the secondary result is completely dropped. How about
> intializing the replace result with, eg., -1 and then print an empty
> string from replace_dev_result2string instead?

I assume the secondary result won't be filled at all if the ioctl
itself failed due to some reason(indicated by errno). But yes, it's
relatively safer to check the replace result instead of dropping it
completely.

I'll send v2 out.

Thanks for your reivew!

Eryu

[PATCH v2 1/2] btrfs-progs: only report internal dev replace result if there's a result

[Eryu Guan]

If BTRFS_IOC_DEV_REPLACE ioctl failed, args.result usually won't be
updated by the ioctl.

And the arg has been initialized with 0, the result is always 0, which
is BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR, and the resulting error
message looks confusing:

ERROR: ioctl(DEV_REPLACE_START) failed on "/mnt/btrfs": No such file or directory, no error

But in case there's an internal result returned in future, don't drop
the result completely, instead print dev replace result message only
if the result is updated by a failed ioctl call.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---

I didn't update replace_dev_result2string() function, which seems not
necessary here. And I personally still prefer v1 :)

v2:
- don't drop the result completely but print result if it's got updated

 cmds-replace.c | 44 ++++++++++++++++++++++++++++++++------------
 ioctl.h        |  1 +
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index 9fe7ad8..bfcc161 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -184,12 +184,17 @@ static int cmd_start_replace(int argc, char **argv)
 
 	/* check for possible errors before backgrounding */
 	status_args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS;
+	status_args.result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT;
 	ret = ioctl(fdmnt, BTRFS_IOC_DEV_REPLACE, &status_args);
 	if (ret) {
 		fprintf(stderr,
-			"ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s, %s\n",
-			path, strerror(errno),
-			replace_dev_result2string(status_args.result));
+			"ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s",
+			path, strerror(errno));
+		if (status_args.result != BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT)
+			fprintf(stderr, ", %s\n",
+				replace_dev_result2string(status_args.result));
+		else
+			fprintf(stderr, "\n");
 		goto leave_with_error;
 	}
 
@@ -301,13 +306,18 @@ static int cmd_start_replace(int argc, char **argv)
 	}
 
 	start_args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_START;
+	start_args.result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT;
 	ret = ioctl(fdmnt, BTRFS_IOC_DEV_REPLACE, &start_args);
 	if (do_not_background) {
 		if (ret) {
 			fprintf(stderr,
-				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s, %s\n",
-				path, strerror(errno),
-				replace_dev_result2string(start_args.result));
+				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s",
+				path, strerror(errno));
+			if (start_args.result != BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT)
+				fprintf(stderr, ", %s\n",
+					replace_dev_result2string(start_args.result));
+			else
+				fprintf(stderr, "\n");
 
 			if (errno == EOPNOTSUPP)
 				fprintf(stderr,
@@ -402,11 +412,16 @@ static int print_replace_status(int fd, const char *path, int once)
 
 	for (;;) {
 		args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS;
+		args.result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT;
 		ret = ioctl(fd, BTRFS_IOC_DEV_REPLACE, &args);
 		if (ret) {
-			fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s, %s\n",
-				path, strerror(errno),
-				replace_dev_result2string(args.result));
+			fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_STATUS) failed on \"%s\": %s",
+				path, strerror(errno));
+			if (args.result != BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT)
+				fprintf(stderr, ", %s\n",
+					replace_dev_result2string(args.result));
+			else
+				fprintf(stderr, "\n");
 			return ret;
 		}
 
@@ -550,13 +565,18 @@ static int cmd_cancel_replace(int argc, char **argv)
 	}
 
 	args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_CANCEL;
+	args.result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT;
 	ret = ioctl(fd, BTRFS_IOC_DEV_REPLACE, &args);
 	e = errno;
 	close_file_or_dir(fd, dirstream);
 	if (ret) {
-		fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_CANCEL) failed on \"%s\": %s, %s\n",
-			path, strerror(e),
-			replace_dev_result2string(args.result));
+		fprintf(stderr, "ERROR: ioctl(DEV_REPLACE_CANCEL) failed on \"%s\": %s",
+			path, strerror(e));
+		if (args.result != BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT)
+			fprintf(stderr, ", %s\n",
+				replace_dev_result2string(args.result));
+		else
+			fprintf(stderr, "\n");
 		return 1;
 	}
 	if (args.result == BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED) {
diff --git a/ioctl.h b/ioctl.h
index f0fc060..df9e4b7 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -141,6 +141,7 @@ struct btrfs_ioctl_dev_replace_status_params {
 #define BTRFS_IOCTL_DEV_REPLACE_CMD_START			0
 #define BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS			1
 #define BTRFS_IOCTL_DEV_REPLACE_CMD_CANCEL			2
+#define BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT		-1
 #define BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR			0
 #define BTRFS_IOCTL_DEV_REPLACE_RESULT_NOT_STARTED		1
 #define BTRFS_IOCTL_DEV_REPLACE_RESULT_ALREADY_STARTED		2
-- 
1.8.3.1