| Message ID | 1572849196-21775-5-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: global verbose and quiet option | expand |
On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote: > /* > + * Global verbose option for the sub-commands > + */ > +#define HELPINFO_GLOBAL_OPTIONS_HEADER \ > + "", \ > + "Global options:" > +#define HELPINFO_INSERT_VERBOSE \ > + "-v|--verbose show verbose output" > +#define HELPINFO_INSERT_QUIET \ > + "-q|--quiet run the command quietly" > + > +/* > * Special marker in the help strings that will preemptively insert the global > * options and then continue with the following text that possibly follows > * after the regular options I've realized there's more magic around the global options, because currently the --format option depends on the subcommand definition thus it can't be a static text like you do with the definition of HELPINFO_GLOBAL_OPTIONS_HEADER. There's a special keyword that gets replaced, the verbose/quite options don't need that so it's just the plain textual definition/description. As this is a simple fixup s/HELPINFO_GLOBAL_OPTIONS_HEADER/HELPINFO_INSERT_GLOBALS/, hold on with resending I might find more things or fix it myself.
On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote: > + case 'v': > + bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; This code gets repeated and it's not IMO simple enough to be copy-pasted around. Eg. bconf_be_verbose() and eventually bconf_be_quiet(). > + break; > + case 'q': > + bconf.verbose = 0; > + break; > default: > fprintf(stderr, "Unknown global option: %s\n", > argv[optind - 1]); > --- a/common/help.h > +++ b/common/help.h > @@ -53,6 +53,17 @@ > "-t|--tbytes show sizes in TiB, or TB with --si" > > /* > + * Global verbose option for the sub-commands > + */ > +#define HELPINFO_GLOBAL_OPTIONS_HEADER \ > + "", \ > + "Global options:" > +#define HELPINFO_INSERT_VERBOSE \ > + "-v|--verbose show verbose output" increase output verbosity > +#define HELPINFO_INSERT_QUIET \ > + "-q|--quiet run the command quietly" print only errors > + > +/* > * Special marker in the help strings that will preemptively insert the global > * options and then continue with the following text that possibly follows > * after the regular options > --- a/common/utils.h > +++ b/common/utils.h > @@ -122,6 +122,9 @@ void print_all_devices(struct list_head *devices); > */ > struct btrfs_config { > unsigned int output_format; > + > + /* -1:unset 0:quiet >0:verbose */ Instead of the constants, please add some defines for the unset and default states. Maybe also for quiet. > + int verbose; > }; > extern struct btrfs_config bconf;
On 11/15/19 12:08 AM, David Sterba wrote: > On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote: >> /* >> + * Global verbose option for the sub-commands >> + */ >> +#define HELPINFO_GLOBAL_OPTIONS_HEADER \ >> + "", \ >> + "Global options:" >> +#define HELPINFO_INSERT_VERBOSE \ >> + "-v|--verbose show verbose output" >> +#define HELPINFO_INSERT_QUIET \ >> + "-q|--quiet run the command quietly" >> + >> +/* >> * Special marker in the help strings that will preemptively insert the global >> * options and then continue with the following text that possibly follows >> * after the regular options > > I've realized there's more magic around the global options, because > currently the --format option depends on the subcommand definition thus > it can't be a static text like you do with the definition of > HELPINFO_GLOBAL_OPTIONS_HEADER. > > There's a special keyword that gets replaced, the verbose/quite options > don't need that so it's just the plain textual definition/description. > > As this is a simple fixup > s/HELPINFO_GLOBAL_OPTIONS_HEADER/HELPINFO_INSERT_GLOBALS/, hold on with > resending I might find more things or fix it myself. > As v2 is in progress I shall fix this. Thanks, Anand
On 11/15/19 12:08 AM, David Sterba wrote: > On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote: >> /* >> + * Global verbose option for the sub-commands >> + */ >> +#define HELPINFO_GLOBAL_OPTIONS_HEADER \ >> + "", \ >> + "Global options:" >> +#define HELPINFO_INSERT_VERBOSE \ >> + "-v|--verbose show verbose output" >> +#define HELPINFO_INSERT_QUIET \ >> + "-q|--quiet run the command quietly" >> + >> +/* >> * Special marker in the help strings that will preemptively insert the global >> * options and then continue with the following text that possibly follows >> * after the regular options > > I've realized there's more magic around the global options, because > currently the --format option depends on the subcommand definition thus > it can't be a static text like you do with the definition of > HELPINFO_GLOBAL_OPTIONS_HEADER. > > There's a special keyword that gets replaced, the verbose/quite options > don't need that so it's just the plain textual definition/description. > > As this is a simple fixup > s/HELPINFO_GLOBAL_OPTIONS_HEADER/HELPINFO_INSERT_GLOBALS/, hold on with > resending I might find more things or fix it myself. > But there is one problem, HELPINFO_INSERT_GLOBALS is already defined as.. Global options: --format TYPE where TYPE is: text (ref: common/help.c do_usage_one_command()) Albeit all commands support --format text by default. But no sub-command is using the HELPINFO_INSERT_GLOBALS in its usage. Looks like its a good idea to separate title and the options, like #define HELPINFO_INSERT_GLOBALS "Global options:" #define HELPINGO_INSERT_FORMAT "--format TYPE" As at the moment I am not sure if its safe to declare that all sub-commands will support --format json (whenever we do that). So with the defines split as above, in each sub-command-usage we shall do.. ----------------------------------------- diff --git a/cmds/filesystem.c b/cmds/filesystem.c index 4f22089abeaa..f4dba38b4c17 100644 --- a/cmds/filesystem.c +++ b/cmds/filesystem.c @@ -631,6 +631,10 @@ static const char * const cmd_filesystem_show_usage[] = { "-m|--mounted show only mounted btrfs", HELPINFO_UNITS_LONG, "If no argument is given, structure of all present filesystems is shown.", + HELPINFO_INSERT_GLOBALS, + HELPINFO_INSERT_FORMAT, + HELPINFO_INSERT_VERBOSE, + HELPINFO_INSERT_QUIET, NULL ----------------------------------------- Thanks, Anand
On 11/15/19 11:58 PM, David Sterba wrote: > On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote: >> + case 'v': >> + bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; > > This code gets repeated and it's not IMO simple enough to be copy-pasted > around. Eg. bconf_be_verbose() and eventually bconf_be_quiet(). I was just concerned- it will make life of other developers difficult, IMO too much abstraction in the code is almost like learning a new programming language for the new person looking at the code. For example fstests. To write patch for fstests you need to learn about a lot of helpers, defines and functions and filters specific to fstests but you wouldn't have had this problem if the fstests abstractions were limited and if it embraced open-code style. Just my 1c. For now I have added bconf_be_verbose() and bconf_be_quiet() it looks neat as below, + case 'v': + bconf_be_verbose(); + break; + case 'q': + bconf_be_quiet(); + break; >> + break; >> + case 'q': >> + bconf.verbose = 0; >> + break; >> default: >> fprintf(stderr, "Unknown global option: %s\n", >> argv[optind - 1]); >> --- a/common/help.h >> +++ b/common/help.h >> @@ -53,6 +53,17 @@ >> "-t|--tbytes show sizes in TiB, or TB with --si" >> >> /* >> + * Global verbose option for the sub-commands >> + */ >> +#define HELPINFO_GLOBAL_OPTIONS_HEADER \ >> + "", \ >> + "Global options:" >> +#define HELPINFO_INSERT_VERBOSE \ >> + "-v|--verbose show verbose output" > > increase output verbosity fixed. >> +#define HELPINFO_INSERT_QUIET \ >> + "-q|--quiet run the command quietly" > print only errors fixed. >> + >> +/* >> * Special marker in the help strings that will preemptively insert the global >> * options and then continue with the following text that possibly follows >> * after the regular options >> --- a/common/utils.h >> +++ b/common/utils.h >> @@ -122,6 +122,9 @@ void print_all_devices(struct list_head *devices); >> */ >> struct btrfs_config { >> unsigned int output_format; >> + >> + /* -1:unset 0:quiet >0:verbose */ > > Instead of the constants, please add some defines for the unset and > default states. Maybe also for quiet. Fixed. +#define BTRFS_BCONF_QUIET 0 +#define BTRFS_BCONF_UNSET -1 Thanks for the valuable comments. Anand >> + int verbose; >> }; >> extern struct btrfs_config bconf;
On Tue, Nov 19, 2019 at 11:36:59AM +0800, Anand Jain wrote: > > As this is a simple fixup > > s/HELPINFO_GLOBAL_OPTIONS_HEADER/HELPINFO_INSERT_GLOBALS/, hold on with > > resending I might find more things or fix it myself. > > > > But there is one problem, HELPINFO_INSERT_GLOBALS is already defined > as.. > Global options: > --format TYPE where TYPE is: text > > (ref: common/help.c do_usage_one_command()) > > Albeit all commands support --format text by default. > > But no sub-command is using the HELPINFO_INSERT_GLOBALS in its usage. > > Looks like its a good idea to separate title and the options, like > #define HELPINFO_INSERT_GLOBALS "Global options:" > #define HELPINGO_INSERT_FORMAT "--format TYPE" Yeah, makes sense to split it, right now the format does not bring anything so that will be better done in a major release some time in the future when more commands have json output. > As at the moment I am not sure if its safe to declare that all > sub-commands will support --format json (whenever we do that). No, right now json output should not be declared anywhere. > So with the defines split as above, in each sub-command-usage > we shall do.. > > ----------------------------------------- > diff --git a/cmds/filesystem.c b/cmds/filesystem.c > index 4f22089abeaa..f4dba38b4c17 100644 > --- a/cmds/filesystem.c > +++ b/cmds/filesystem.c > @@ -631,6 +631,10 @@ static const char * const > cmd_filesystem_show_usage[] = { > "-m|--mounted show only mounted btrfs", > HELPINFO_UNITS_LONG, > "If no argument is given, structure of all present filesystems > is shown.", > + HELPINFO_INSERT_GLOBALS, > + HELPINFO_INSERT_FORMAT, > + HELPINFO_INSERT_VERBOSE, > + HELPINFO_INSERT_QUIET, Sounds ok.
On Tue, Nov 19, 2019 at 01:07:05PM +0800, Anand Jain wrote: > On 11/15/19 11:58 PM, David Sterba wrote: > > On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote: > >> + case 'v': > >> + bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; > > > > This code gets repeated and it's not IMO simple enough to be copy-pasted > > around. Eg. bconf_be_verbose() and eventually bconf_be_quiet(). > > I was just concerned- it will make life of other developers difficult, > IMO too much abstraction in the code is almost like learning a new > programming language for the new person looking at the code. > For example fstests. To write patch for fstests you need to > learn about a lot of helpers, defines and functions and filters > specific to fstests but you wouldn't have had this problem if the > fstests abstractions were limited and if it embraced open-code style. > Just my 1c. Yes it takes time to learn the abstractions but then it makes a lot of things easier and allows to think about what to do and not necessarily how. In a clean codebase there are enough examples to follow or copy & adapt, I don't think it's a big deal and that it's normal and expected when one works on various code bases.
On 11/20/19 12:51 AM, David Sterba wrote: > On Tue, Nov 19, 2019 at 11:36:59AM +0800, Anand Jain wrote: >>> As this is a simple fixup >>> s/HELPINFO_GLOBAL_OPTIONS_HEADER/HELPINFO_INSERT_GLOBALS/, hold on with >>> resending I might find more things or fix it myself. >>> >> >> But there is one problem, HELPINFO_INSERT_GLOBALS is already defined >> as.. >> Global options: >> --format TYPE where TYPE is: text >> >> (ref: common/help.c do_usage_one_command()) >> >> Albeit all commands support --format text by default. >> >> But no sub-command is using the HELPINFO_INSERT_GLOBALS in its usage. >> >> Looks like its a good idea to separate title and the options, like >> #define HELPINFO_INSERT_GLOBALS "Global options:" >> #define HELPINGO_INSERT_FORMAT "--format TYPE" > > Yeah, makes sense to split it, right now the format does not bring > anything so that will be better done in a major release some time in the > future when more commands have json output. Ok. In v2 I have split HELPINFO_GLOBAL_OPTIONS_HEADER into HELPINFO_INSERT_GLOBALS and HELPINGO_INSERT_FORMAT as above. Thanks, Anand >> As at the moment I am not sure if its safe to declare that all >> sub-commands will support --format json (whenever we do that). > > No, right now json output should not be declared anywhere. > >> So with the defines split as above, in each sub-command-usage >> we shall do.. >> >> ----------------------------------------- >> diff --git a/cmds/filesystem.c b/cmds/filesystem.c >> index 4f22089abeaa..f4dba38b4c17 100644 >> --- a/cmds/filesystem.c >> +++ b/cmds/filesystem.c >> @@ -631,6 +631,10 @@ static const char * const >> cmd_filesystem_show_usage[] = { >> "-m|--mounted show only mounted btrfs", >> HELPINFO_UNITS_LONG, >> "If no argument is given, structure of all present filesystems >> is shown.", >> + HELPINFO_INSERT_GLOBALS, >> + HELPINFO_INSERT_FORMAT, >> + HELPINFO_INSERT_VERBOSE, >> + HELPINFO_INSERT_QUIET, > > Sounds ok. >
diff --git a/btrfs.c b/btrfs.c index 6c8aabe24dc8..a97bc1858390 100644 --- a/btrfs.c +++ b/btrfs.c @@ -27,7 +27,7 @@ #include "common/box.h" static const char * const btrfs_cmd_group_usage[] = { - "btrfs [--help] [--version] [--format <format>] <group> [<group>...] <command> [<args>]", + "btrfs [--help] [--version] [--format <format>] [-v|--verbose] [--quiet] <group> [<group>...] <command> [<args>]", NULL }; @@ -248,6 +248,8 @@ static int handle_global_options(int argc, char **argv) { "version", no_argument, NULL, OPT_VERSION }, { "format", required_argument, NULL, OPT_FORMAT }, { "full", no_argument, NULL, OPT_FULL }, + { "verbose", no_argument, NULL, 'v' }, + { "quiet", no_argument, NULL, 'q' }, { NULL, 0, NULL, 0} }; int shift; @@ -259,7 +261,7 @@ static int handle_global_options(int argc, char **argv) while (1) { int c; - c = getopt_long(argc, argv, "+", long_options, NULL); + c = getopt_long(argc, argv, "+vq", long_options, NULL); if (c < 0) break; @@ -270,6 +272,12 @@ static int handle_global_options(int argc, char **argv) case OPT_FORMAT: handle_output_format(optarg); break; + case 'v': + bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; + break; + case 'q': + bconf.verbose = 0; + break; default: fprintf(stderr, "Unknown global option: %s\n", argv[optind - 1]); @@ -310,6 +318,14 @@ static void handle_special_globals(int shift, int argc, char **argv) cmd_execute(&cmd_struct_version, argc, argv); exit(0); } + + for (i = 0; i < shift; i++) + if (strcmp(argv[i], "--verbose") == 0) + bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++; + + for (i = 0; i < shift; i++) + if (strcmp(argv[i], "--quiet") == 0) + bconf.verbose = 0; } static const struct cmd_group btrfs_cmd_group = { diff --git a/common/help.h b/common/help.h index 01dfc68a7c8d..2618f31de712 100644 --- a/common/help.h +++ b/common/help.h @@ -53,6 +53,17 @@ "-t|--tbytes show sizes in TiB, or TB with --si" /* + * Global verbose option for the sub-commands + */ +#define HELPINFO_GLOBAL_OPTIONS_HEADER \ + "", \ + "Global options:" +#define HELPINFO_INSERT_VERBOSE \ + "-v|--verbose show verbose output" +#define HELPINFO_INSERT_QUIET \ + "-q|--quiet run the command quietly" + +/* * Special marker in the help strings that will preemptively insert the global * options and then continue with the following text that possibly follows * after the regular options diff --git a/common/messages.c b/common/messages.c index 0e5694ecd467..410630ffdd6d 100644 --- a/common/messages.c +++ b/common/messages.c @@ -17,6 +17,7 @@ #include <stdio.h> #include <stdarg.h> #include "common/messages.h" +#include "common/utils.h" __attribute__ ((format (printf, 1, 2))) void __btrfs_warning(const char *fmt, ...) @@ -75,3 +76,19 @@ int __btrfs_error_on(int condition, const char *fmt, ...) return 1; } + +__attribute__ ((format (printf, 2, 3))) +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); +} diff --git a/common/messages.h b/common/messages.h index 596047948fef..f802a9413265 100644 --- a/common/messages.h +++ b/common/messages.h @@ -96,3 +96,6 @@ __attribute__ ((format (printf, 2, 3))) int __btrfs_error_on(int condition, const char *fmt, ...); #endif + +__attribute__ ((format (printf, 2, 3))) +void pr_verbose(int level, const char *fmt, ...); diff --git a/common/utils.c b/common/utils.c index 2cf15c333f6b..c2c6d0af0efc 100644 --- a/common/utils.c +++ b/common/utils.c @@ -1649,6 +1649,7 @@ u8 rand_u8(void) void btrfs_config_init(void) { bconf.output_format = CMD_FORMAT_TEXT; + bconf.verbose = -1; } /* Returns total size of main memory in bytes, -1UL if error. */ diff --git a/common/utils.h b/common/utils.h index 0ef1d6e89c2b..8774194f4e9d 100644 --- a/common/utils.h +++ b/common/utils.h @@ -122,6 +122,9 @@ void print_all_devices(struct list_head *devices); */ struct btrfs_config { unsigned int output_format; + + /* -1:unset 0:quiet >0:verbose */ + int verbose; }; extern struct btrfs_config bconf;
Add btrfs(8) global --verbose and --quiet command options to show verbose or no output from the sub-commands. By introducing global a %bconf::verbose memeber to transpire the same down to the sub-command. Further the added helper function pr_verbose() helps to logs the verbose messages, based on the state of the %bconf::verbose. And further HELPINFO_ defines are provides for the usage. Suggested-by: David Sterba <dsterba@suse.com> Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v1.1: Fix typo in HELPINFO_INSERT_QUIET Drop stale #include <stdbool.h> we don't need it because %bconf.verbose is now declared as int. btrfs.c | 20 ++++++++++++++++++-- common/help.h | 11 +++++++++++ common/messages.c | 17 +++++++++++++++++ common/messages.h | 3 +++ common/utils.c | 1 + common/utils.h | 3 +++ 6 files changed, 53 insertions(+), 2 deletions(-)