Re: [PATCH v9 3/3] xenpm: Add get-core-temp subcommand

["Anthony PERARD" <anthony.perard@xxxxxxxxxx>]

On Mon, Mar 16, 2026 at 02:34:09PM +0000, Teddy Astie wrote:
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index e4902d2e82..37f484b362 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> +static void get_core_temp(int argc, char *argv[])
> +{
> +    int temp = -1, cpu = -1;
> +    unsigned int socket;
> +    bool has_data = false;
> +
> +    if ( argc > 0 )
> +        parse_cpuid(argv[0], &cpu);
> +
> +    if ( cpu != -1 )
> +    {
> +        if ( !fetch_dts_temp(xc_handle, cpu, false, &temp) )
> +            printf("CPU%d: %d°C\n", cpu, temp);
> +        else
> +        {
> +            fprintf(stderr, "Unable to fetch temperature (%d - %s)\n",
> +                    errno, strerror(errno));
> +            printf("No data\n");

What is this "no data" for? There's already two clues which says that
there's nothing to print, the error message on stderr, and the non-zero
exit value.

> +            exit(EXIT_FAILURE);
> +        }
> +        return;
> +    }
> +
> +    /* Per socket measurement */
> +    for ( socket = 0, cpu = 0; cpu < max_cpu_nr;
> +          socket++, cpu += physinfo.cores_per_socket * 
> physinfo.threads_per_core )
> +    {
> +        if ( fetch_dts_temp(xc_handle, cpu, true, &temp) )

Here, you deal with the error return by fetch_dts_temp() first, then the
success, but in the previous block (cpu=all) you do the opposite. Also,
here the success isn't even in the else part, but after. Could you
choose one style to be consistent?

I think I prefer to deal with the error first, so like here.

> +        {
> +            fprintf(stderr,
> +                    "[Package%u] Unable to fetch temperature (%d - %s)\n",
> +                    cpu, errno, strerror(errno));
> +            continue;

If we got an error one one package, aren't we likely to got more error?
Is it worth to keep trying on the next package?

> +        }
> +
> +        has_data = true;
> +        printf("Package%u: %d°C\n", socket, temp);
> +    }
> +
> +    if ( has_data )
> +        /* Avoid inserting a trailing line if we have nothing */
> +        printf("\n");
> +
> +    for ( cpu = 0; cpu < max_cpu_nr; cpu += physinfo.threads_per_core )
> +    {
> +        if ( fetch_dts_temp(xc_handle, cpu, false, &temp) )
> +        {
> +            fprintf(stderr, "[CPU%d] Unable to fetch temperature (%d - 
> %s)\n",
> +                    cpu, errno, strerror(errno));
> +            continue;
> +        }
> +
> +        has_data = true;
> +        printf("CPU%d: %d°C\n", cpu, temp);
> +    }
> +
> +    if ( !has_data )
> +    {
> +        printf("No data\n");

Another "no data".

> +        exit(EXIT_FAILURE);
> +    }
> +}
> +

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech