| Message ID | 1412761349-32721-1-git-send-email-guaneryu@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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? -- 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
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 -- 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
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) {
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(-)