Re: [PATCH] grep: correctly initialize help-all option

From: René Scharfe <l.s.r@web.de>
Date: Apr 9, 2015, 21:55 UTC
Message-ID: <5526F535.4020407@web.de>
In-Reply-To: <1428586916-22679-1-git-send-email-ps@pks.im>

Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:

> The "help-all" option is being initialized with a wrong value.
> While being semantically wrong this can also cause a gcc
> segmentation fault on ARMv7 hardfloat platforms with a hardened
> toolchain. Fix this by initializing with the correct value.

Thanks for your report and patch.  A few comments:

Sign-off?  (See "Sign your work" in Documentation/SubmittingPatches)

Show 17 quoted lines

> ---
>   builtin/grep.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index abc4400..c0bf005 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>   			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
>   		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
>   			 N_("allow calling of grep(1) (ignored by this build)")),
> -		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
> +		{ OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),
>   		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>   		OPT_END()
>   	};

help_callback() returns -1 immediately, IOW the value pointer is never used anyway. So why does your change make a difference? *puzzled*

We could pass NULL instead, as in builtin/show-ref.c, which would make it clear that the pointer is just a dummy.

Q: Why does the callback exist in the first place?  A: parse_options() 
handles -h and --help-all automatically by showing the list of available 
options.  This is good for most commands for consistency's sake, but bad 
for commands that want to use -h for something else.  That's why it can 
be turned off with the flag PARSE_OPT_NO_INTERNAL_HELP, but that flag 
disables the long option as well.  It's added back by grep and show-ref 
explicitly, to at least provide --help-all consistently across all commands.

We could solve this problem centrally by checking for -h and --help-all only after looking through the struct option list supplied to parse_options_step() instead of before and getting rid of the then unneeded callbacks for --help-all in grep and show-ref. Are there any downsides to that approach?

René