Message ID | 20201216034240.2029-1-realwakka@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs-progs: filesystem-resize: make output more readable | expand |
On Wed, Dec 16, 2020 at 03:42:40AM +0000, Sidong Yang wrote: > This patch make output of filesystem-resize command more readable and > give detail information for users. This patch provides more information > about filesystem like below. > > Before: > Resize '/mnt' of '1:-1G' > > After: > Resize device id 1 (/dev/vdb) from 4.00GiB to 3.00GiB > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > --- > cmds/filesystem.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 60 insertions(+), 1 deletion(-) > > diff --git a/cmds/filesystem.c b/cmds/filesystem.c > index fac612b2..53e775b7 100644 > --- a/cmds/filesystem.c > +++ b/cmds/filesystem.c > @@ -1084,6 +1084,14 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > int ret; > struct stat st; > bool enqueue = false; > + struct btrfs_ioctl_fs_info_args fi_args; > + struct btrfs_ioctl_dev_info_args *di_args = NULL; > + char newsize[256]; > + char sign; > + u64 inc_bytes; > + u64 res_bytes; > + int i, devid, dev_idx; > + const char *res_str; > > optind = 0; > while (1) { > @@ -1142,7 +1150,58 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > return 1; > } > > - printf("Resize '%s' of '%s'\n", path, amount); > + ret = get_fs_info(path, &fi_args, &di_args); > + if (ret) > + error("unable to retrieve fs info"); The helper 'error' is to just print the message so the code has to change flow to an exit otherwise it would continue, which is what we don't want here. > + > + if (!fi_args.num_devices) > + error("num_devices = 0"); Same and everywhere below. Also the error message is too cryptic, think that there's a human reading that so it should say what's the error, like "No devices found". Which would be a weird and likely impossible error anyway but it's good that it's handled. > + > + ret = sscanf(amount, "%d:%255s", &devid, newsize); > + > + if (ret != 2) > + error("invalid format"); I'm not sure this covers all the possibilities the resize format provides. The "%d:" part is not mandatory and there doesn't need to be ":" at all, eg when it's "max" or any number. There are some examples in manual page of btrfs-filesystem so would be good if we have at least that covered by tests.
On Fri, Jan 15, 2021 at 12:48:59AM +0100, David Sterba wrote: Hi David, Thanks fore review. > On Wed, Dec 16, 2020 at 03:42:40AM +0000, Sidong Yang wrote: > > This patch make output of filesystem-resize command more readable and > > give detail information for users. This patch provides more information > > about filesystem like below. > > > > Before: > > Resize '/mnt' of '1:-1G' > > > > After: > > Resize device id 1 (/dev/vdb) from 4.00GiB to 3.00GiB > > > > Signed-off-by: Sidong Yang <realwakka@gmail.com> > > --- > > cmds/filesystem.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/cmds/filesystem.c b/cmds/filesystem.c > > index fac612b2..53e775b7 100644 > > --- a/cmds/filesystem.c > > +++ b/cmds/filesystem.c > > @@ -1084,6 +1084,14 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > > int ret; > > struct stat st; > > bool enqueue = false; > > + struct btrfs_ioctl_fs_info_args fi_args; > > + struct btrfs_ioctl_dev_info_args *di_args = NULL; > > + char newsize[256]; > > + char sign; > > + u64 inc_bytes; > > + u64 res_bytes; > > + int i, devid, dev_idx; > > + const char *res_str; > > > > optind = 0; > > while (1) { > > @@ -1142,7 +1150,58 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > > return 1; > > } > > > > - printf("Resize '%s' of '%s'\n", path, amount); > > + ret = get_fs_info(path, &fi_args, &di_args); > > + if (ret) > > + error("unable to retrieve fs info"); > > The helper 'error' is to just print the message so the code has to > change flow to an exit otherwise it would continue, which is what we > don't want here. Thanks, I didn't know about error(). I'll fix it. > > > + > > + if (!fi_args.num_devices) > > + error("num_devices = 0"); > > Same and everywhere below. Also the error message is too cryptic, think > that there's a human reading that so it should say what's the error, > like "No devices found". Which would be a weird and likely impossible > error anyway but it's good that it's handled. I have to fix the error messages for users to understand what's the error. > > > + > > + ret = sscanf(amount, "%d:%255s", &devid, newsize); > > + > > + if (ret != 2) > > + error("invalid format"); > > I'm not sure this covers all the possibilities the resize format > provides. The "%d:" part is not mandatory and there doesn't need to be > ":" at all, eg when it's "max" or any number. > > There are some examples in manual page of btrfs-filesystem so would be > good if we have at least that covered by tests. Okay, I'll make it to handle various cases. Thanks! Sidong
diff --git a/cmds/filesystem.c b/cmds/filesystem.c index fac612b2..53e775b7 100644 --- a/cmds/filesystem.c +++ b/cmds/filesystem.c @@ -1084,6 +1084,14 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, int ret; struct stat st; bool enqueue = false; + struct btrfs_ioctl_fs_info_args fi_args; + struct btrfs_ioctl_dev_info_args *di_args = NULL; + char newsize[256]; + char sign; + u64 inc_bytes; + u64 res_bytes; + int i, devid, dev_idx; + const char *res_str; optind = 0; while (1) { @@ -1142,7 +1150,58 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, return 1; } - printf("Resize '%s' of '%s'\n", path, amount); + ret = get_fs_info(path, &fi_args, &di_args); + if (ret) + error("unable to retrieve fs info"); + + if (!fi_args.num_devices) + error("num_devices = 0"); + + ret = sscanf(amount, "%d:%255s", &devid, newsize); + + if (ret != 2) + error("invalid format"); + + dev_idx = -1; + for(i = 0; i < fi_args.num_devices; i++) { + if (di_args[i].devid == devid) { + dev_idx = i; + break; + } + } + + if (dev_idx < 0) + error("cannot find devid : %d", devid); + + if (!strcmp(newsize, "max")) { + res_str = "max"; + } else { + if (strlen(newsize) < 3) + error("invalid format"); + + sign = newsize[0]; + if (sign != '-' && sign != '+') + error("invalid format"); + + inc_bytes = parse_size(&newsize[1]); + + res_bytes = 0; + if (sign == '+') + res_bytes = di_args[0].total_bytes + inc_bytes; + else if (sign == '-') + res_bytes = di_args[0].total_bytes - inc_bytes; + else + error("invalid format"); + + res_str = pretty_size_mode(res_bytes, UNITS_DEFAULT); + } + + printf("Resize device id %d (%s) from %s to %s\n", devid, di_args[dev_idx].path, + pretty_size_mode(di_args[dev_idx].total_bytes, UNITS_DEFAULT), + res_str); + + free(di_args); + memset(&args, 0, sizeof(args)); strncpy_null(args.name, amount); res = ioctl(fd, BTRFS_IOC_RESIZE, &args);
This patch make output of filesystem-resize command more readable and give detail information for users. This patch provides more information about filesystem like below. Before: Resize '/mnt' of '1:-1G' After: Resize device id 1 (/dev/vdb) from 4.00GiB to 3.00GiB Signed-off-by: Sidong Yang <realwakka@gmail.com> --- cmds/filesystem.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-)