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

Anand Jain
2019-10-22T01:54:53Z

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