Re: [PATCH v2 2/2] xenpm: Print message for disabled commands

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2024 21:59, Jason Andryuk wrote:
> xenpm get-cpufreq-states currently just prints no output when cpufreq is
> disabled or HWP is running.  Have it print an appropriate message.  The
> cpufreq disabled one mirros the cpuidle disabled one.
> 
> cpufreq disabled:
> $ xenpm get-cpufreq-states
> Either Xen cpufreq is disabled or no valid information is registered!
> 
> Under HWP:
> $ xenpm get-cpufreq-states
> P-State information not supported.  Try get-cpufreq-average or start.
> 
> Also allow xenpm to handle EOPNOTSUPP from the pmstat hypercalls.
> EOPNOTSUPP is returned when HWP is active in some cases and allows the
> differentiation from cpufreq being disabled.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Largely okay, but a number of cosmetic remarks / nits:

> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -362,7 +362,15 @@ static int show_pxstat_by_cpuid(xc_interface *xc_handle, 
> int cpuid)
>  
>      ret = get_pxstat_by_cpuid(xc_handle, cpuid, &pxstatinfo);
>      if ( ret )
> +    {
> +        if ( ret == -ENODEV )
> +            fprintf(stderr,
> +                    "Either Xen cpufreq is disabled or no valid information 
> is registered!\n");
> +       else if ( ret == -EOPNOTSUPP )

One too few blanks for indentation.

> +            fprintf(stderr,
> +                    "P-State information not supported.  Try 
> get-cpufreq-average or start.\n");

Especially the "or start" part reads odd without any quotation.

> @@ -382,9 +390,11 @@ void pxstat_func(int argc, char *argv[])
>      {
>          /* show pxstates on all cpus */
>          int i;
> -        for ( i = 0; i < max_cpu_nr; i++ )
> -            if ( show_pxstat_by_cpuid(xc_handle, i) == -ENODEV )
> +        for ( i = 0; i < max_cpu_nr; i++ ) {

This file tries to follow hypervisor style, so the brace ought to
go on its own line. While there may I ask that you also add the
missing blank line (separating declaration from statements)? This
then also applies ...

> +            int ret = show_pxstat_by_cpuid(xc_handle, i);
> +            if ( ret == -ENODEV || ret == -EOPNOTSUPP )

... between these two new lines.

>                  break;
> +     }

Hard tab?

> @@ -473,7 +483,8 @@ static void signal_int_handler(int signo)
>                  }
>      }
>  
> -    if ( get_pxstat_by_cpuid(xc_handle, 0, NULL) != -ENODEV )
> +    ret = get_pxstat_by_cpuid(xc_handle, 0, NULL);
> +    if ( ret != -ENODEV && ret != -EOPNOTSUPP )

While looking odd, I can see why it wants to be this way.

Jan