Re: [RFC PATCH 00/14] btrfs-progs: global-verbose option

From: Anand Jain <anand.jain@oracle.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org
Subject: Re: [RFC PATCH 00/14] btrfs-progs: global-verbose option
Date: Tue, 22 Oct 2019 09:54:41 +0800	[thread overview]
Message-ID: <daf2cb74-64bb-66bf-9b08-9f07076eacc8@oracle.com> (raw)
In-Reply-To: <20191021161256.GR3001@twin.jikos.cz>

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.
> 