Re: [PATCH v1.1 04/18] btrfs-progs: add global verbose and quiet options and helper functions

[Anand Jain <anand.jain@oracle.com>]

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;