Re: git: 68bff4a07e3f - main - Allow GEOM utilities to specify a -v option.

Hi Kirk,

On 29 Oct 2021, at 22:12, Kristof Provost wrote:
> On 29 Oct 2021, at 7:52, Kirk McKusick wrote:
>> The branch main has been updated by mckusick:
>>
>> URL: 
>> https://cgit.FreeBSD.org/src/commit/?id=68bff4a07e3fa6c30a0c0ff6cf5f0bef95dcbd72
>>
>> commit 68bff4a07e3fa6c30a0c0ff6cf5f0bef95dcbd72
>> Author:     Kirk McKusick <mckusick@FreeBSD.org>
>> AuthorDate: 2021-10-29 05:49:48 +0000
>> Commit:     Kirk McKusick <mckusick@FreeBSD.org>
>> CommitDate: 2021-10-29 05:50:50 +0000
>>
>>     Allow GEOM utilities to specify a -v option.
>>
>>     Geom utilities (geli(8), glabel(8), gmirror(8), gpart(8), 
>> gmirror(8),
>>     gmountver(8), etc) all use the geom(8) utility as their back end
>>     to process their commands and pass them into the kernel. Creating
>>     a new utility requires no more than filling out a template 
>> describing
>>     the commands and arguments that the utility supports. Consider 
>> the
>>     specification for the very simple gmountver(8) utility:
>>
>>     struct g_command class_commands[] = {
>>             { "create", G_FLAG_VERBOSE | G_FLAG_LOADKLD, NULL,
>>                 {
>>                     G_OPT_SENTINEL
>>                 },
>>                 "[-v] prov ..."
>>             },
>>             { "destroy", G_FLAG_VERBOSE, NULL,
>>                 {
>>                     { 'f', "force", NULL, G_TYPE_BOOL },
>>                     G_OPT_SENTINEL
>>                 },
>>                 "[-fv] name"
>>             },
>>             G_CMD_SENTINEL
>>     };
>>
>>     It has just two commands of its own: "create" and "destroy" along
>>     with the four standard commands "list", "status", "load", and
>>     "unload" provided by the base geom(8) utility. The base geom(8)
>>     utility allows each command to use the G_FLAG_VERBOSE flag to 
>> specify
>>     that a command should accept the -v flag and when the -v flag is
>>     given the utility prints "Done." if the command completes 
>> successfully.
>>     In the above example, both of the commands set the 
>> G_FLAG_VERBOSE,
>>     so have the -v option available. In addition the "destroy" 
>> command
>>     accepts the -f boolean flag to force the destruction.
>>
>>     If the "destroy" command wanted to also print out verbose 
>> information,
>>     it would need to explicitly declare its intent by adding a line:
>>
>>                     { 'v', "verbose", NULL, G_TYPE_BOOL },
>>
>>     Before this change, the geom utility would silently ignore the 
>> above
>>     line in the configuration file, so it was impossible for the 
>> utility
>>     to know that the -v flag had been set on the command. With this
>>     change a geom command can explicitly specify a -v option with a
>>     line as given above and handle it as it would any other option. 
>> If
>>     both a -v option and G_FLAG_VERBOSE are specified for a command
>>     then both types of verbose information will be output when that
>>     command is run with -v.
>>
>>     MFC after:    1 week
>>     Sponsored by: Netflix
>> ---
>>  sbin/geom/core/geom.c | 19 ++++++++++++-------
>>  sbin/geom/core/geom.h | 13 +++++++++++++
>>  2 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/sbin/geom/core/geom.c b/sbin/geom/core/geom.c
>> index 58b33a067700..2e0d8683df49 100644
>> --- a/sbin/geom/core/geom.c
>> +++ b/sbin/geom/core/geom.c
>
>> @@ -440,7 +445,7 @@ set_flags(struct g_command *cmd)
>>  {
>>  	unsigned flags = 0;
>>
>> -	if ((cmd->gc_flags & G_FLAG_VERBOSE) != 0 && verbose)
>> +	if ((cmd->gc_flags & G_FLAG_VERBOSE) != 0)
>>  		flags |= G_FLAG_VERBOSE;
>>
>>  	return (flags);
>
> Given https://reviews.freebsd.org/D32736 I wonder if the removal of 
> the verbose check here was correct.
>
> If I'm reading this code right we now always set the verbose flag for 
> any subcommand that supports it (i.e. has G_FLAG_VERBOSE set).
>
> That leads to commands such as glabel always acting as if '-v' was 
> specified.
>
> The set_flags() output is only used if g_func is set, which isn't the 
> case in any of the examples in the commit message, so I wonder if that 
> case was overlooked.
>
Have you had a chance to look at this? It’s causing multiple failures 
in the geom regression tests.

Thanks,
Kristof