| Message ID | 1574678357-22222-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs-progs: global verbose and quiet option | expand |
Gentle ping. Regds, Anand
David, I wonder if could this be integrated? Thanks, Anand On 25/11/19 6:39 PM, Anand Jain wrote: > v1.1->v2: > Mainly: > . Splits define HELPINFO_INSERT_GLOBALS from --format option and > . Rename HELPINFO_GLOBAL_OPTIONS_HEADER to HELPINFO_INSERT_GLOBALS > . Create and use helper bconf_be_verbose() and bconf_be_quiet(). > . Use gobal bconf.verbose where possible and drop local verbose argument passing. > . In some patches the bconf.verbose initialization wasn't necessary so drop it. > Some small fixes as mentioned in the individual patch. > > v1->v1.1: > . Fix typo in HELPINFO_INSERT_QUIET. > . Remove #include <stdbool.h> where its no more required. > (was needed when %bconf.verbose was declared as bool). > . Use pr_verbose(-1,..) instead of all conditions printf() > . Use pr_verbose(1,..) instead of pr_verbose(true,..) > > verbosity sample code as in v1.1 > > global init > ----------- > bconf.verbose = -1; //-1:default, 0:quiet, >1:verbose > > at global options > ----------------- > case 'v': > bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; > break; > case 'q': > bconf.verbose = 0; > break; > > sub-command init > ---------------- > For send/receive only (special cases, default verbosity is 1): > > if (bconf.verbose < 0) > bconf.verbose = 1; > else if (bconf.verbose > 0) > bconf.verbose++; > > Non-send/receive: > default verbosity is 0 (ref: cmds/rescue.c) > if (bconf.verbose < 0) > bconf.verbose = 0; > > at sub-command options > ---------------------- > case 'v': > bconf.verbose++; > break; > case 'q': > bconf.verbose = 0; > break; > > > pr_verbose() > ------------ > /* > * level -1: prints message unless bconf.verbose == 0; > * level 0: quiet > * level >0: prints message only if <= bconf.verbose > */ > void pr_verbose(int level, const char *fmt, ...) > { > va_list args; > > if (level == 0 || bconf.verbose == 0) > return; > > if (level > bconf.verbose) > return; > > va_start(args, fmt); > vfprintf(stdout, fmt, args); > va_end(args); > } > > > RFC->v1: > .. Adds --quiet option to the global btrfs(8) command. > .. Used global struct bconf. > .. Refactored pr_verbose(), accepts level (int) as argument, so now the > sub-command can specify the verbose level at which the particular > verbose messages has to be printed. > .. Added global help defines. > > Kindly note the following:- > > 1. > There are certain sub-commands which does not have any verbose output > or quiet output. However if the global options were used with those > sub-commands then the command shall not report any usage error. Or > my question is should it error out.? For example: > (with the patch) btrfs --verbose device ready /dev/sdb > actually there isn't any verbose output but we won't error out. > Similarly, > (without the patch) btrfs send -vvvvv will not show usage error > as well. > So I believe this is fine. IMO. > > 2. > There is slight difference in output when global options are used > as compared to the output using the same sequence of options at the > sub-command level. For example: > > btrfs send -v -q -v is-equal-to btrfs send > But same sequence in the global option > btrfs -v -q -v send is-not-equal-to btrfs send > but is-equal-to btrfs -v send or btrfs send -v. > (similarly applies to receive as well). > > which IMO is fair expectation as -v is ending last. > > > RFC: > This patch set brings --verbose option to the top level btrfs command, > such as 'btrfs --verbose'. With this we don't have to add or remember > verbose option at the sub-commands level. > > As there are already verbose options to 11 sub-commands as listed > below [1][2]. So the top level --verbose option here takes care to transpire > verbose request from the top level to the sub-command level for 9 (not 11) > sub-commands as in [1] as of now. > > This patch is RFC still for the following two reasons (comments appreciated). > > 1. > The sub-commands as in [2] uses multi-level compile time verbose option, > such as %g_verbose = 0 (quite), %g_verbose = 1 (default), %g_verbose > 1 > (real-verbose). And verbose at default is also part the .out files in > fstests. So it needs further discussions on how to handle the multi- > level verbose option using the global verbose option, and so sub- > commands in [2] are untouched. > > 2. > These patch has been unit-tested individually. > These patches does not alter the verbose output. > But it fixes the indentation in the command's help output, which may be > used in fstests and btrfs-progs/tests and their verification is pending > still, which I am planning to do it before v1. > > [1] > btrfs subvolume delete --help > -v|--verbose verbose output of operations > btrfs filesystem defragment --help > -v be verbose > btrfs balance start --help > -v|--verbose be verbose > btrfs balance status --help > -v|--verbose be verbose > btrfs rescue chunk-recover --help > -v Verbose mode > btrfs rescue super-recover --help > -v Verbose mode > btrfs restore --help > -v|--verbose verbose > btrfs inspect-internal inode-resolve --help > -v verbose mode > btrfs inspect-internal logical-resolve --help > -v verbose mode > > [2] > btrfs send --help > -v|--verbose enable verbose output to stderr, each occurrence of > btrfs receive --help > -v increase verbosity about performed action > > > > Anand Jain (16): > btrfs-progs: split global help HELPINFO_INSERT_GLOBALS > btrfs-progs: add global verbose and quiet options and helper functions > btrfs-progs: send: use global verbose and quiet options > btrfs-progs: receive: use global verbose and quiet options > btrfs-progs: subvolume delete: use global verbose option > btrfs-progs: filesystem defragment: use global verbose option > btrfs-progs: balance start: use global verbose option > btrfs-progs: balance status: use global verbose option > btrfs-progs: rescue chunk-recover: use global verbose option > btrfs-progs: rescue super-recover: use global verbose option > btrfs-progs: restore: use global verbose option > btrfs-progs: inspect-internal inode-resolve: use global verbose > btrfs-progs: inspect-internal logical-resolve: use global verbose > option > btrfs-progs: refactor btrfs_scan_devices() to accept verbose argument > btrfs-progs: device scan: add verbose option > btrfs-progs: device scan: add quiet option > > btrfs.c | 20 ++++++++++-- > cmds/balance.c | 14 ++++---- > cmds/device.c | 7 ++-- > cmds/filesystem.c | 14 ++++---- > cmds/inspect.c | 41 +++++++++++------------ > cmds/receive.c | 80 +++++++++++++++++++++++++-------------------- > cmds/rescue-chunk-recover.c | 9 +++-- > cmds/rescue-super-recover.c | 7 ++-- > cmds/rescue.c | 18 ++++++---- > cmds/rescue.h | 4 +-- > cmds/restore.c | 53 +++++++++++++----------------- > cmds/send.c | 33 ++++++++++++------- > cmds/subvolume.c | 28 ++++++++-------- > common/device-scan.c | 3 +- > common/device-scan.h | 2 +- > common/help.c | 4 +-- > common/help.h | 9 ++++- > common/messages.c | 25 ++++++++++++++ > common/messages.h | 3 ++ > common/utils.c | 16 ++++++++- > common/utils.h | 11 +++++++ > disk-io.c | 2 +- > 22 files changed, 245 insertions(+), 158 deletions(-) >
On Tue, Jan 14, 2020 at 02:14:24PM +0800, Anand Jain wrote:
> I wonder if could this be integrated?
Yes, when I get to it through the pile of other patches.
On 14/1/20 7:40 PM, David Sterba wrote: > On Tue, Jan 14, 2020 at 02:14:24PM +0800, Anand Jain wrote: >> I wonder if could this be integrated? > > Yes, when I get to it through the pile of other patches. > Can this get into the upcoming release. Thanks, Anand
David, A ping on this series. Thanks, Anand On 28/3/20 1:45 pm, Anand Jain wrote: > On 14/1/20 7:40 PM, David Sterba wrote: >> On Tue, Jan 14, 2020 at 02:14:24PM +0800, Anand Jain wrote: >>> I wonder if could this be integrated? >> >> Yes, when I get to it through the pile of other patches. >> > > Can this get into the upcoming release. > > Thanks, Anand
On Wed, May 20, 2020 at 06:01:28PM +0800, Anand Jain wrote:
> A ping on this series.
So I want to merge the series but I think it's unfinished. There are
mostly --verbose options added, while --quiet is only for send and
receive. My first test was 'btrfs -q subvolume create' and 'delete' and
that -q did not work is surprising. The -v/-q options need to come in
the same release.
Also, many options that have their own --verbose option should update
the help text to note that it's an alias of the global.
I'm going to take another look today, the scope of the change might be
too big to do in one go so some incremental steps might be needed.
> My first test was 'btrfs -q subvolume create' and 'delete' and > that -q did not work is surprising. The -v/-q options need to come in > the same release. Ah. I thought rest of the commands shall adopt -q option progressively. So its not unfinished. Anyway I can fix them now. > Also, many options that have their own --verbose option should update > the help text to note that it's an alias of the global. Hmm. Yep. Fixing them. > I'm going to take another look today, the scope of the change might be > too big to do in one go so some incremental steps might be needed. Sure. Thanks. Anand
On Fri, Jun 05, 2020 at 06:12:30PM +0800, Anand Jain wrote: > > > My first test was 'btrfs -q subvolume create' and 'delete' and > > that -q did not work is surprising. The -v/-q options need to come in > > the same release. > > Ah. I thought rest of the commands shall adopt -q option progressively. > So its not unfinished. Anyway I can fix them now. From use case point of view it is unfinished, adding options for verbosity without the corresponding quieting option just does not make sense. The global options were my suggestion but so far I don't think we've reached common understanding what's needed to implement that.
> Also, many options that have their own --verbose option should update > the help text to note that it's an alias of the global. > > I'm going to take another look today, the scope of the change might be > too big to do in one go so some incremental steps might be needed. As global verbose depends on the local verbose to log messages, if local verbose used stderr then the global verbose uses the stderr. For example patch: btrfs-progs: send: use global verbose and quiet options ------------------ ./btrfs send --help <snip> -v|--verbose enable verbose output to stderr, each occurrence of this option increases verbosity -q|--quiet suppress all messages, except errors Global options: -v|--verbose increase output verbosity -q|--quiet print only errors ------------------ IMO verbose should be stdout. Should it be ok to change to stdout as it is under verbose? Thanks.
v1.1->v2: Mainly: . Splits define HELPINFO_INSERT_GLOBALS from --format option and . Rename HELPINFO_GLOBAL_OPTIONS_HEADER to HELPINFO_INSERT_GLOBALS . Create and use helper bconf_be_verbose() and bconf_be_quiet(). . Use gobal bconf.verbose where possible and drop local verbose argument passing. . In some patches the bconf.verbose initialization wasn't necessary so drop it. Some small fixes as mentioned in the individual patch. v1->v1.1: . Fix typo in HELPINFO_INSERT_QUIET. . Remove #include <stdbool.h> where its no more required. (was needed when %bconf.verbose was declared as bool). . Use pr_verbose(-1,..) instead of all conditions printf() . Use pr_verbose(1,..) instead of pr_verbose(true,..) verbosity sample code as in v1.1 global init ----------- bconf.verbose = -1; //-1:default, 0:quiet, >1:verbose at global options ----------------- case 'v': bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; break; case 'q': bconf.verbose = 0; break; sub-command init ---------------- For send/receive only (special cases, default verbosity is 1): if (bconf.verbose < 0) bconf.verbose = 1; else if (bconf.verbose > 0) bconf.verbose++; Non-send/receive: default verbosity is 0 (ref: cmds/rescue.c) if (bconf.verbose < 0) bconf.verbose = 0; at sub-command options ---------------------- case 'v': bconf.verbose++; break; case 'q': bconf.verbose = 0; break; pr_verbose() ------------ /* * level -1: prints message unless bconf.verbose == 0; * level 0: quiet * level >0: prints message only if <= bconf.verbose */ void pr_verbose(int level, const char *fmt, ...) { va_list args; if (level == 0 || bconf.verbose == 0) return; if (level > bconf.verbose) return; va_start(args, fmt); vfprintf(stdout, fmt, args); va_end(args); } RFC->v1: . Adds --quiet option to the global btrfs(8) command. . Used global struct bconf. . Refactored pr_verbose(), accepts level (int) as argument, so now the sub-command can specify the verbose level at which the particular verbose messages has to be printed. . Added global help defines. Kindly note the following:- 1. There are certain sub-commands which does not have any verbose output or quiet output. However if the global options were used with those sub-commands then the command shall not report any usage error. Or my question is should it error out.? For example: (with the patch) btrfs --verbose device ready /dev/sdb actually there isn't any verbose output but we won't error out. Similarly, (without the patch) btrfs send -vvvvv will not show usage error as well. So I believe this is fine. IMO. 2. There is slight difference in output when global options are used as compared to the output using the same sequence of options at the sub-command level. For example: btrfs send -v -q -v is-equal-to btrfs send But same sequence in the global option btrfs -v -q -v send is-not-equal-to btrfs send but is-equal-to btrfs -v send or btrfs send -v. (similarly applies to receive as well). which IMO is fair expectation as -v is ending last. RFC: This patch set brings --verbose option to the top level btrfs command, such as 'btrfs --verbose'. With this we don't have to add or remember verbose option at the sub-commands level. As there are already verbose options to 11 sub-commands as listed below [1][2]. So the top level --verbose option here takes care to transpire verbose request from the top level to the sub-command level for 9 (not 11) sub-commands as in [1] as of now. This patch is RFC still for the following two reasons (comments appreciated). 1. The sub-commands as in [2] uses multi-level compile time verbose option, such as %g_verbose = 0 (quite), %g_verbose = 1 (default), %g_verbose > 1 (real-verbose). And verbose at default is also part the .out files in fstests. So it needs further discussions on how to handle the multi- level verbose option using the global verbose option, and so sub- commands in [2] are untouched. 2. These patch has been unit-tested individually. These patches does not alter the verbose output. But it fixes the indentation in the command's help output, which may be used in fstests and btrfs-progs/tests and their verification is pending still, which I am planning to do it before v1. [1] btrfs subvolume delete --help -v|--verbose verbose output of operations btrfs filesystem defragment --help -v be verbose btrfs balance start --help -v|--verbose be verbose btrfs balance status --help -v|--verbose be verbose btrfs rescue chunk-recover --help -v Verbose mode btrfs rescue super-recover --help -v Verbose mode btrfs restore --help -v|--verbose verbose btrfs inspect-internal inode-resolve --help -v verbose mode btrfs inspect-internal logical-resolve --help -v verbose mode [2] btrfs send --help -v|--verbose enable verbose output to stderr, each occurrence of btrfs receive --help -v increase verbosity about performed action Anand Jain (16): btrfs-progs: split global help HELPINFO_INSERT_GLOBALS btrfs-progs: add global verbose and quiet options and helper functions btrfs-progs: send: use global verbose and quiet options btrfs-progs: receive: use global verbose and quiet options btrfs-progs: subvolume delete: use global verbose option btrfs-progs: filesystem defragment: use global verbose option btrfs-progs: balance start: use global verbose option btrfs-progs: balance status: use global verbose option btrfs-progs: rescue chunk-recover: use global verbose option btrfs-progs: rescue super-recover: use global verbose option btrfs-progs: restore: use global verbose option btrfs-progs: inspect-internal inode-resolve: use global verbose btrfs-progs: inspect-internal logical-resolve: use global verbose option btrfs-progs: refactor btrfs_scan_devices() to accept verbose argument btrfs-progs: device scan: add verbose option btrfs-progs: device scan: add quiet option btrfs.c | 20 ++++++++++-- cmds/balance.c | 14 ++++---- cmds/device.c | 7 ++-- cmds/filesystem.c | 14 ++++---- cmds/inspect.c | 41 +++++++++++------------ cmds/receive.c | 80 +++++++++++++++++++++++++-------------------- cmds/rescue-chunk-recover.c | 9 +++-- cmds/rescue-super-recover.c | 7 ++-- cmds/rescue.c | 18 ++++++---- cmds/rescue.h | 4 +-- cmds/restore.c | 53 +++++++++++++----------------- cmds/send.c | 33 ++++++++++++------- cmds/subvolume.c | 28 ++++++++-------- common/device-scan.c | 3 +- common/device-scan.h | 2 +- common/help.c | 4 +-- common/help.h | 9 ++++- common/messages.c | 25 ++++++++++++++ common/messages.h | 3 ++ common/utils.c | 16 ++++++++- common/utils.h | 11 +++++++ disk-io.c | 2 +- 22 files changed, 245 insertions(+), 158 deletions(-)