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

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.

Best regards,
Kristof