Re: [PATCH v3 13/14 RESEND] xenpm: Add set-cpufreq-hwp subcommand

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.05.2023 14:59, Jason Andryuk wrote:
> On Mon, May 8, 2023 at 7:56 AM Jan Beulich <jbeulich@xxxxxxxx> 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