| Message ID | 1571652082-25982-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs-progs: global-verbose option | expand |
On Mon, Oct 21, 2019 at 06:01:08PM +0800, Anand Jain wrote: > 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. The idea is to unify all verbosity options. Default is 1, 0 is for quiet (only errors are printed), the rest is up to the commands what to print on the higher levels. > 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. The indentation does not need to be changed if the glboal options are split from the per-command, like --- usage: btrfs subvolume delete [options] <subvolume> [<subvolume>...] Delete subvolume(s) Delete subvolumes from the filesystem. The corresponding directory is removed instantly but the data blocks are removed later. The deletion does not involve full commit by default due to performance reasons (as a consequence, the subvolume may appear again after a crash). Use one of the --commit options to wait until the operation is safely stored on the media. -c|--commit-after wait for transaction commit at the end of the operation -C|--commit-each wait for transaction commit after deleting each subvolume Global options: -v|--verbose show verbose output --- Some commands can have long option names or the argument names make it long in some cases, the global options could stay indented. I think visually it'll be ok. We can introduce some way to automatically format the options and help texts so we don't have to adjust them manually each time, but this would be more intrusive and can be done later. With the global verbose option there shouldbe also -q|--quiet. Both short and long versions should be available for all commands. So the help would look like: --- Global options: -v|--verbose verbose output, repeat for more verbosity -q|--quiet print only errors --- In code this looks like: "", "-c|--commit-after wait for transaction commit at the end of the operation", "-C|--commit-each wait for transaction commit after deleting each subvolume", HELPINFO_GLOBAL_OPTIONS_HEADER, HELPINFO_INSERT_VERBOSE, NULL #define HELPINFO_GLOBAL_OPTIONS_HEADER \ "", \ "Global options:" and HELPINFO_INSERT_VERBOSE also contains the quiet option. The global option value is stored in 'btrfs_config_init bconf', so everything can access it directly. Thanks for working on this, I'll have more comments on v2 as I probably forgot a few more things to do, the above is the base for all further changes.
On 10/22/19 12:12 AM, David Sterba wrote: > On Mon, Oct 21, 2019 at 06:01:08PM +0800, Anand Jain wrote: >> 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. > > The idea is to unify all verbosity options. Default is 1, 0 is for quiet > (only errors are printed), the rest is up to the commands what to print > on the higher levels. As of now verbosity level is a compile time option. [3] [3] ------- cmds/send.c 51 /* 52 * Default is 1 for historical reasons, changing may break scripts that expect 53 * the 'At subvol' message. 54 */ 55 static int g_verbose = 1; -------- >> 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. > > The indentation does not need to be changed if the glboal options are > split from the per-command, like > > --- > usage: btrfs subvolume delete [options] <subvolume> [<subvolume>...] > > Delete subvolume(s) > > Delete subvolumes from the filesystem. The corresponding directory > is removed instantly but the data blocks are removed later. > The deletion does not involve full commit by default due to > performance reasons (as a consequence, the subvolume may appear again > after a crash). Use one of the --commit options to wait until the > operation is safely stored on the media. > > -c|--commit-after wait for transaction commit at the end of the operation > -C|--commit-each wait for transaction commit after deleting each subvolume > > Global options: > -v|--verbose show verbose output > --- Oh split it. ok. Makes sense. > Some commands can have long option names or the argument names make it > long in some cases, the global options could stay indented. I think > visually it'll be ok. We can introduce some way to automatically format > the options and help texts so we don't have to adjust them manually each > time, but this would be more intrusive and can be done later. ok. But my pertaining question is if the sub-command verbose option should still remain? if no I will be happy to take it out as the same verbose will anyway be activated using the global verbose option. > With the global verbose option there shouldbe also -q|--quiet. Both > short and long versions should be available for all commands. So the > help would look like: > > --- > Global options: > -v|--verbose verbose output, repeat for more verbosity > -q|--quiet print only errors > --- > > In code this looks like: > > "", > "-c|--commit-after wait for transaction commit at the end of the operation", > "-C|--commit-each wait for transaction commit after deleting each subvolume", > HELPINFO_GLOBAL_OPTIONS_HEADER, > HELPINFO_INSERT_VERBOSE, > NULL > > #define HELPINFO_GLOBAL_OPTIONS_HEADER \ > "", \ > "Global options:" > > and HELPINFO_INSERT_VERBOSE also contains the quiet option. > > The global option value is stored in 'btrfs_config_init bconf', so > everything can access it directly. Oh ok. In the above code-snap [3]. g_verbose = 0 and g_verbose = 1 can be mapped to the global -q|--quite and --verbose respectively. But any idea what to do with g_verbose > 1? which we support in send.c and receive.c. And in defrag which the patch [4] removed it. [4] [RFC PATCH 09/14] btrfs-progs: restore: delete unreachable code Another way is btrfs [--quite] [--verbose[=n]] n=1 default n=2 verbose Can't imagine anything better. Thanks for helping to shape this. Anand > Thanks for working on this, I'll have more comments on v2 as I probably > forgot a few more things to do, the above is the base for all further > changes. >
On Tue, Oct 22, 2019 at 09:54:41AM +0800, Anand Jain wrote: > >> 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. > > > > The idea is to unify all verbosity options. Default is 1, 0 is for quiet > > (only errors are printed), the rest is up to the commands what to print > > on the higher levels. > > As of now verbosity level is a compile time option. [3] > > [3] > ------- > cmds/send.c > > 51 /* > 52 * Default is 1 for historical reasons, changing may break scripts > that expect > 53 * the 'At subvol' message. > 54 */ > 55 static int g_verbose = 1; > -------- All the specific options would need to be unified while also maintaining backward compatibility, like the above comment. Fortunatelly, if we set default verbosity to 1, the only thing to do here will be to convert it to the global config. > > Some commands can have long option names or the argument names make it > > long in some cases, the global options could stay indented. I think > > visually it'll be ok. We can introduce some way to automatically format > > the options and help texts so we don't have to adjust them manually each > > time, but this would be more intrusive and can be done later. > > ok. But my pertaining question is if the sub-command verbose option > should still remain? if no I will be happy to take it out as the same > verbose will anyway be activated using the global verbose option. For backward compatibility, where the per-command verbosity option exists, it must continue, but will be an alias to the global option. This is the awkward part but that' the cost of compatibility. > > With the global verbose option there shouldbe also -q|--quiet. Both > > short and long versions should be available for all commands. So the > > help would look like: > > > > --- > > Global options: > > -v|--verbose verbose output, repeat for more verbosity > > -q|--quiet print only errors > > --- > > > > In code this looks like: > > > > "", > > "-c|--commit-after wait for transaction commit at the end of the operation", > > "-C|--commit-each wait for transaction commit after deleting each subvolume", > > HELPINFO_GLOBAL_OPTIONS_HEADER, > > HELPINFO_INSERT_VERBOSE, > > NULL > > > > #define HELPINFO_GLOBAL_OPTIONS_HEADER \ > > "", \ > > "Global options:" > > > > and HELPINFO_INSERT_VERBOSE also contains the quiet option. > > > > The global option value is stored in 'btrfs_config_init bconf', so > > everything can access it directly. > > Oh ok. > > In the above code-snap [3]. > > g_verbose = 0 and g_verbose = 1 can be mapped to the global > -q|--quite and --verbose respectively. Yes, that's right > But any idea what to do with g_verbose > 1? which we support > in send.c and receive.c. And in defrag which the patch [4] removed it. The verbosity option can be usually repeated and increasing the level, so $ btrfs -vvv send subvol would be the same as $ btrfs send -vvv subvol > [4] > [RFC PATCH 09/14] btrfs-progs: restore: delete unreachable code > > Another way is > > btrfs [--quite] [--verbose[=n]] > > n=1 default > n=2 verbose I'm not sure I've seen the '-v=n' way of specifying verbosity and would rather avoid optional arugment. The code hadling -v would do case OPT_VERBOSE: bconf.verbose++; break; case OPT_QUIET: bconf.verbose = 0; break;
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 (14): btrfs-progs: add global verbose helper functions btrfs-progs: migrate subvolume delete to global verbose btrfs-progs: migrate filesystem defragment to global verbose btrfs-progs: migrate btrfs balance start to global verbose btrfs-progs: migrate balance status to global verbose btrfs-progs: fix help, show long option in balance start and status btrfs-progs: migrate rescue chunk-recover to global verbose btrfs-progs: migrate rescue super-recover to global verbose btrfs-progs: restore: delete unreachable code btrfs-progs: migrate restore to global verbose btrfs-progs: migrate inspect-internal inode-resolve to global verbose btrfs-progs: migrate inspect-internal logical-resolve to global verbose btrfs-progs: refactor btrfs_scan_devices() to accept verbose argument btrfs-progs: enable verbose for btrfs device scan btrfs.c | 12 ++++++-- cmds/balance.c | 29 +++++++++--------- cmds/device.c | 2 +- cmds/filesystem.c | 27 ++++++++--------- cmds/inspect.c | 36 +++++++++++----------- cmds/rescue.c | 22 +++++++------- cmds/restore.c | 86 +++++++++++++++++++++------------------------------- cmds/subvolume.c | 35 +++++++++++---------- common/device-scan.c | 4 ++- common/device-scan.h | 2 +- common/help.h | 8 +++++ common/messages.c | 19 ++++++++++++ common/messages.h | 5 +++ common/utils.c | 2 +- disk-io.c | 2 +- 15 files changed, 155 insertions(+), 136 deletions(-)