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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 30/03/2026 à 15:11, Anthony PERARD a écrit :
> 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.
>

The "no data" could be removed now that the information is transmitted
differently.

>> +            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.
>

Looks good to me.

>> +        {
>> +            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".
>

It could be removed as well.

---

I wonder if we can remove the "has_data" boolean by making any (aside
package temperature if it's not supported) failure cause a
exit(EXIT_FAILURE) instead ?

We lose the empty line between Package and CPU temperature though, as we
need to add it only if package temperature exists.

>> +        exit(EXIT_FAILURE);
>> +    }
>> +}
>> +
>
> Thanks,
>
>
> --
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
>
>
>



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech