[v1.1,04/18] btrfs-progs: add global verbose and quiet options and helper functions
diff mbox series

Message ID 1572849196-21775-5-git-send-email-anand.jain@oracle.com
State New
Headers show
Series
  • btrfs-progs: global verbose and quiet option
Related show

Commit Message

Anand Jain Nov. 4, 2019, 6:33 a.m. UTC
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(-)

Comments

David Sterba Nov. 14, 2019, 4:08 p.m. UTC | #1
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.
David Sterba Nov. 15, 2019, 3:58 p.m. UTC | #2
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;
Anand Jain Nov. 19, 2019, 2:44 a.m. UTC | #3
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
Anand Jain Nov. 19, 2019, 3:36 a.m. UTC | #4
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
Anand Jain Nov. 19, 2019, 5:07 a.m. UTC | #5
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;
David Sterba Nov. 19, 2019, 4:51 p.m. UTC | #6
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.
David Sterba Nov. 19, 2019, 5:02 p.m. UTC | #7
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.
Anand Jain Nov. 25, 2019, 10:36 a.m. UTC | #8
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.
>

Patch
diff mbox series

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;