Re: [PATCH v1.1 00/18] btrfs-progs: global verbose and quiet option

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

On 11/16/19 12:11 AM, David Sterba wrote:
> On Mon, Nov 04, 2019 at 02:32:58PM +0800, Anand Jain wrote:
>> 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
> 
> Please user integer numbers for patch revisions, no need to mark patches
> that haven't changed, the overall summary of changes can mention what
> changed where if needed.

  Ok got it.
  (I kind of didn't want to integer++ unless it fixes review comments).

>> pr_verbose()
>> ------------
>> /*
>>   * level -1: prints message unless bconf.verbose == 0;
>>   * level  0: quiet
> 
> Are we ever going to call the function with level == 0?

   Yes. For example in the patch

    [PATCH v1.1 13/18] btrfs-progs: restore: use global verbose option

--------------
@@ -375,8 +374,7 @@ static int copy_one_extent(struct btrfs_root *root, 
int fd,
         if (compress == BTRFS_COMPRESS_NONE)
                 bytenr += offset;

-       if (verbose && offset)
-               printf("offset is %Lu\n", offset);
+       pr_verbose(offset ? 1 : 0, "offset is %Lu\n", offset);
--------------



>>   * 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);
>> }
>>
>> 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.
> 
> Yes this is fine.
> 
>> 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.
> 
> Agreed, hopefully the wild combinations of -v and -q are not too common.
> 
> The patchset looks good, though it needs the small fixups like the
> global header or the helper macros, the core of the changes is there.
> That should be good for 5.4.
> 
> The first version merged should bring the support for global verbosity
> options, then we can gradually convert all fprintf/printf to the helpers
> and add new printfs with higher verbosity levels.
> 

Yes. Will do.

Thanks, Anand