On 22.05.2023 14:59, Jason Andryuk wrote:
> On Mon, May 8, 2023 at 7:56 AM Jan Beulich <jbeul...@suse.com> wrote:
>> On 01.05.2023 21:30, Jason Andryuk wrote:
>>> +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid,
>>> + int argc, char *argv[])
>>> +{
>>> + int i = 0;
>>> +
>>> + if ( argc < 1 ) {
>>> + fprintf(stderr, "Missing arguments\n");
>>> + return -1;
>>> + }
>>> +
>>> + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 )
>>> + {
>>> + i++;
>>> + }
>>
>> I don't think you need the earlier patch and the separate helper:
>> Whether a CPU number is present can be told by checking
>> isdigit(argv[i][0]).
>
>> Hmm, yes, there is "all", but your help text doesn't mention it and
>> since you're handling a variable number of arguments anyway, there's
>> not need for anyone to say "all" - they can simply omit the optional
>> argument.
>
> Most xenpm commands take "all" or a numeric cpuid, so I intended to be
> consistent with them. That was the whole point of
> parse_cpuid_non_fatal() - to reuse the existing parsing code for
> consistency.
>
> I didn't read the other help text carefully enough to see that the
> numeric cpuid and "all" handling was repeated.
>
> For consistency, I would retain parse_cpuid_non_fatal() and expand the
> help text. If you don't want that, I'll switch to isdigit(argv[i][0])
> and have the omission of a digit indicate all CPUs as you suggest.
> Just let me know what you want.
While I don't want to push you towards something you don't like yourself,
my view on the "all" has been "Why did they introduce that?" It makes
some sense when it's a placeholder to avoid needing to deal with a
variable number of arguments, but already that doesn't apply to all the
pre-existing operations. Note how many functions already have
if ( argc > 0 )
parse_cpuid(argv[0], &cpuid);
and {en,dis}able-turbo-mode don't properly mention "all" in their help
text either.
Jan
