[PATCH v8 1/2] libxc: Report consistent errors in xc_resource_op

[PATCH v8 1/2] libxc: Report consistent errors in xc_resource_op

[Teddy Astie]

xc_report_op doesn't update errno in some error conditions.
Make sure it reports -ENOMEM in out of memory errors and -EINVAL
in invalid usages errors.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
v7: Introduced
v8: Use errno to report errors

 tools/libs/ctrl/xc_resource.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/libs/ctrl/xc_resource.c b/tools/libs/ctrl/xc_resource.c
index cb6a97202b..ac1524d1bd 100644
--- a/tools/libs/ctrl/xc_resource.c
+++ b/tools/libs/ctrl/xc_resource.c
@@ -28,7 +28,10 @@ static int xc_resource_op_one(xc_interface *xch, xc_resource_op_t *op)
                                 XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
 
     if ( xc_hypercall_bounce_pre(xch, entries) )
+    {
+        errno = ENOMEM;
         return -1;
+    }
 
     platform_op.cmd = XENPF_resource_op;
     platform_op.u.resource_op.nr_entries = op->nr_entries;
@@ -54,11 +57,15 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
     call_list = xc_hypercall_buffer_alloc(xch, call_list,
                                           sizeof(*call_list) * nr_ops);
     if ( !call_list )
+    {
+        errno = ENOMEM;
         return -1;
+    }
 
     platform_ops = xc_hypercall_buffer_array_create(xch, nr_ops);
     if ( !platform_ops )
     {
+        errno = ENOMEM;
         rc = -1;
         goto out;
     }
@@ -66,6 +73,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
     entries_list = xc_hypercall_buffer_array_create(xch, nr_ops);
     if ( !entries_list )
     {
+        errno = ENOMEM;
         rc = -1;
         goto out;
     }
@@ -81,6 +89,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
                         platform_op, sizeof(xen_platform_op_t));
         if ( !platform_op )
         {
+            errno = ENOMEM;
             rc = -1;
             goto out;
         }
@@ -90,6 +99,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
                    entries, entries_size);
         if ( !entries)
         {
+            errno = ENOMEM;
             rc = -1;
             goto out;
         }
@@ -137,6 +147,7 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops)
     if ( nr_ops > 1 )
         return xc_resource_op_multi(xch, nr_ops, ops);
 
+    errno = EINVAL;
     return -1;
 }
 
-- 
2.53.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH v8 2/2] xenpm: Add get-core-temp subcommand

[Teddy Astie]

get-core-temp allows querying the per-core CPU temperature and
per-package one on processors that supports Digital Temperature Sensors
(most Intel processors; as usual Dom0 drivers cannot work due to
misalignment between Dom0 vCPU and pCPUs).

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
CC: Jan Beulich <jbeulich@suse.com>

v4: https://lore.kernel.org/xen-devel/cover.1766158766.git.teddy.astie@vates.tech/
v5: Removed trailing whitespace.
v6: Report errors through errno and use strerror() to display them
v7:
 - Rename get-intel-temp with get-dts-temp
 - handle properly errno
 - make process return a error code if no data
v8:
 - update Changelog
 - improve error handling
 - rename core-dts-temp with get-core-temp

 CHANGELOG.md       |   2 +
 tools/misc/xenpm.c | 127 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 18f3d10f20..ac2a0412e0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - Support for Bus Lock Threshold on AMD Zen5 and later CPUs, used by Xen to
      mitigate (by rate-limiting) the system wide impact of an HVM guest
      misusing atomic instructions.
+   - Introduce get-core-temp to xenpm to query CPU temperatures on Intel
+     platforms.

 ### Removed
  - On x86:
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 682d092479..981d3ec519 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -32,11 +32,14 @@

 #include <xen-tools/common-macros.h>

+#include <xen/asm/msr-index.h>
+
 #define MAX_PKG_RESIDENCIES 12
 #define MAX_CORE_RESIDENCIES 8

 static xc_interface *xc_handle;
 static unsigned int max_cpu_nr;
+static xc_physinfo_t physinfo;

 /* help message */
 void show_help(void)
@@ -93,6 +96,7 @@ void show_help(void)
             "                                           units default to \"us\" if unspecified.\n"
             "                                           truncates un-representable values.\n"
             "                                           0 lets the hardware decide.\n"
+            " get-core-temp          [cpuid]      get CPU temperature for <cpuid> or all (Intel only)\n"
             " start [seconds]                     start collect Cx/Px statistics,\n"
             "                                     output after CTRL-C or SIGINT or several seconds.\n"
             " enable-turbo-mode     [cpuid]       enable Turbo Mode for processors that support it.\n"
@@ -1354,6 +1358,127 @@ void enable_turbo_mode(int argc, char *argv[])
                 errno, strerror(errno));
 }

+static int fetch_dts_temp(xc_interface *xch, uint32_t cpu, bool package, int *temp)
+{
+    xc_resource_entry_t entries[] = {
+        { .idx = package ? MSR_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS },
+        { .idx = MSR_TEMPERATURE_TARGET },
+    };
+    struct xc_resource_op ops = {
+        .cpu = cpu,
+        .entries = entries,
+        .nr_entries = ARRAY_SIZE(entries),
+    };
+    int tjmax;
+
+    int ret = xc_resource_op(xch, 1, &ops);
+
+    switch ( ret )
+    {
+    case 0:
+        /* This CPU isn't online or can't query this MSR */
+        return -1;
+
+    case 1:
+    {
+        /*
+         * The CPU doesn't support MSR_TEMPERATURE_TARGET, we assume it's 100
+         * which is correct aside a few selected Atom CPUs. Check Linux
+         * kernel's coretemp.c for more information.
+         */
+        static bool has_reported_once = false;
+
+        if ( !has_reported_once )
+        {
+            fprintf(stderr, "MSR_TEMPERATURE_TARGET is not supported, assume "
+                            "tjmax = 100, readings may be incorrect.\n");
+            has_reported_once = true;
+        }
+
+        tjmax = 100;
+        break;
+    }
+
+    case 2:
+        tjmax = (entries[1].val >> 16) & 0xff;
+        break;
+
+    default:
+        if ( ret > 0 )
+        {
+            fprintf(stderr, "Got unexpected xc_resource_op return value: %d", ret);
+            errno = EINVAL;
+        }
+        return -1;
+    }
+
+    *temp = tjmax - ((entries[0].val >> 16) & 0xff);
+    return 0;
+}
+
+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");
+            exit(ENODATA);
+        }
+        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) )
+        {
+            fprintf(stderr,
+                    "[Package%u] Unable to fetch temperature (%d - %s)\n",
+                    cpu, errno, strerror(errno));
+            continue;
+        }
+
+        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");
+        exit(ENODATA);
+    }
+}
+
 void disable_turbo_mode(int argc, char *argv[])
 {
     int cpuid = -1;
@@ -1618,12 +1743,12 @@ struct {
     { "set-max-cstate", set_max_cstate_func},
     { "enable-turbo-mode", enable_turbo_mode },
     { "disable-turbo-mode", disable_turbo_mode },
+    { "get-core-temp", get_core_temp },
 };

 int main(int argc, char *argv[])
 {
     int i, ret = 0;
-    xc_physinfo_t physinfo;
     int nr_matches = 0;
     int matches_main_options[ARRAY_SIZE(main_options)];

--
2.53.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v8 2/2] xenpm: Add get-core-temp subcommand

[Jan Beulich]

On 27.02.2026 18:00, Teddy Astie wrote:
> @@ -1354,6 +1358,127 @@ void enable_turbo_mode(int argc, char *argv[])
>                  errno, strerror(errno));
>  }
>  
> +static int fetch_dts_temp(xc_interface *xch, uint32_t cpu, bool package, int *temp)
> +{
> +    xc_resource_entry_t entries[] = {
> +        { .idx = package ? MSR_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS },
> +        { .idx = MSR_TEMPERATURE_TARGET },
> +    };
> +    struct xc_resource_op ops = {
> +        .cpu = cpu,
> +        .entries = entries,
> +        .nr_entries = ARRAY_SIZE(entries),
> +    };
> +    int tjmax;
> +
> +    int ret = xc_resource_op(xch, 1, &ops);
> +
> +    switch ( ret )
> +    {
> +    case 0:
> +        /* This CPU isn't online or can't query this MSR */
> +        return -1;

Further down at the callers of this function you assume errno is set whenever
an error indication is returned. As xc_resource_op() didn't fail, you will
need to synthesize an errno value here.

> +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");
> +            exit(ENODATA);

In how far is using errno values as arguments to exit() a useful thing? (I
think you had it like this before, and I merely forgot to ask.) Yes, I can
see the tool using a number of exit(EINVAL), but I don't understand those
either. This way you can't even document easily what particular exit codes
mean, as the errno values may vary across OSes.

Jan

Re: [PATCH v8 2/2] xenpm: Add get-core-temp subcommand

[Teddy Astie]

Le 02/03/2026 à 17:52, Jan Beulich a écrit :
> On 27.02.2026 18:00, Teddy Astie wrote:
>> @@ -1354,6 +1358,127 @@ void enable_turbo_mode(int argc, char *argv[])
>>                   errno, strerror(errno));
>>   }
>>
>> +static int fetch_dts_temp(xc_interface *xch, uint32_t cpu, bool package, int *temp)
>> +{
>> +    xc_resource_entry_t entries[] = {
>> +        { .idx = package ? MSR_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS },
>> +        { .idx = MSR_TEMPERATURE_TARGET },
>> +    };
>> +    struct xc_resource_op ops = {
>> +        .cpu = cpu,
>> +        .entries = entries,
>> +        .nr_entries = ARRAY_SIZE(entries),
>> +    };
>> +    int tjmax;
>> +
>> +    int ret = xc_resource_op(xch, 1, &ops);
>> +
>> +    switch ( ret )
>> +    {
>> +    case 0:
>> +        /* This CPU isn't online or can't query this MSR */
>> +        return -1;
>
> Further down at the callers of this function you assume errno is set whenever
> an error indication is returned. As xc_resource_op() didn't fail, you will
> need to synthesize an errno value here.
>

ah yes indeed

>> +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");
>> +            exit(ENODATA);
>
> In how far is using errno values as arguments to exit() a useful thing? (I
> think you had it like this before, and I merely forgot to ask.) Yes, I can
> see the tool using a number of exit(EINVAL), but I don't understand those
> either. This way you can't even document easily what particular exit codes
> mean, as the errno values may vary across OSes.
>

I reused the exit(...) pattern used in xenpm, but I'm also fine by
returning simpler errors (like exit(1) or exit(EXIT_FAILURE)).

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 981d3ec519..7d186c4837 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -1377,6 +1377,7 @@ static int fetch_dts_temp(xc_interface *xch,
uint32_t cpu, bool package, int *te
      {
      case 0:
          /* This CPU isn't online or can't query this MSR */
+        errno = ENODATA;
          return -1;

      case 1:
@@ -1434,7 +1435,7 @@ static void get_core_temp(int argc, char *argv[])
              fprintf(stderr, "Unable to fetch temperature (%d - %s)\n",
                      errno, strerror(errno));
              printf("No data\n");
-            exit(ENODATA);
+            exit(EXIT_FAILURE);
          }
          return;
      }
@@ -1475,7 +1476,7 @@ static void get_core_temp(int argc, char *argv[])
      if ( !has_data )
      {
          printf("No data\n");
-        exit(ENODATA);
+        exit(EXIT_FAILURE);
      }
  }

> Jan
>

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v8 2/2] xenpm: Add get-core-temp subcommand

[Jan Beulich]

On 03.03.2026 11:50, Teddy Astie wrote:
> Le 02/03/2026 à 17:52, Jan Beulich a écrit :
>> On 27.02.2026 18:00, Teddy Astie wrote:
>>> @@ -1354,6 +1358,127 @@ void enable_turbo_mode(int argc, char *argv[])
>>>                   errno, strerror(errno));
>>>   }
>>>   
>>> +static int fetch_dts_temp(xc_interface *xch, uint32_t cpu, bool package, int *temp)
>>> +{
>>> +    xc_resource_entry_t entries[] = {
>>> +        { .idx = package ? MSR_PACKAGE_THERM_STATUS : MSR_IA32_THERM_STATUS },
>>> +        { .idx = MSR_TEMPERATURE_TARGET },
>>> +    };
>>> +    struct xc_resource_op ops = {
>>> +        .cpu = cpu,
>>> +        .entries = entries,
>>> +        .nr_entries = ARRAY_SIZE(entries),
>>> +    };
>>> +    int tjmax;
>>> +
>>> +    int ret = xc_resource_op(xch, 1, &ops);
>>> +
>>> +    switch ( ret )
>>> +    {
>>> +    case 0:
>>> +        /* This CPU isn't online or can't query this MSR */
>>> +        return -1;
>>
>> Further down at the callers of this function you assume errno is set whenever
>> an error indication is returned. As xc_resource_op() didn't fail, you will
>> need to synthesize an errno value here.
>>
> 
> ah yes indeed
> 
>>> +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");
>>> +            exit(ENODATA);
>>
>> In how far is using errno values as arguments to exit() a useful thing? (I
>> think you had it like this before, and I merely forgot to ask.) Yes, I can
>> see the tool using a number of exit(EINVAL), but I don't understand those
>> either. This way you can't even document easily what particular exit codes
>> mean, as the errno values may vary across OSes.
>>
> 
> I reused the exit(...) pattern used in xenpm, but I'm also fine by 
> returning simpler errors (like exit(1) or exit(EXIT_FAILURE)).

Anthony, can you please suggest which one better fits the toolstack as a
whole?

Jan

Re: [PATCH v8 2/2] xenpm: Add get-core-temp subcommand

[Anthony PERARD]

On Tue, Mar 03, 2026 at 12:54:28PM +0100, Jan Beulich wrote:
> On 03.03.2026 11:50, Teddy Astie wrote:
> > Le 02/03/2026 à 17:52, Jan Beulich a écrit :
> >> On 27.02.2026 18:00, Teddy Astie wrote:
> >>> +            printf("No data\n");
> >>> +            exit(ENODATA);
> >>
> >> In how far is using errno values as arguments to exit() a useful thing? (I
> >> think you had it like this before, and I merely forgot to ask.) Yes, I can
> >> see the tool using a number of exit(EINVAL), but I don't understand those
> >> either. This way you can't even document easily what particular exit codes
> >> mean, as the errno values may vary across OSes.
> >>
> >
> > I reused the exit(...) pattern used in xenpm, but I'm also fine by
> > returning simpler errors (like exit(1) or exit(EXIT_FAILURE)).
>
> Anthony, can you please suggest which one better fits the toolstack as a
> whole?

There isn't really one, but `exit(EXIT_FAILURE)` would be more
explicit.


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v8 1/2] libxc: Report consistent errors in xc_resource_op

[Anthony PERARD]

On Fri, Feb 27, 2026 at 05:00:05PM +0000, Teddy Astie wrote:
> xc_report_op doesn't update errno in some error conditions.
> Make sure it reports -ENOMEM in out of memory errors and -EINVAL
> in invalid usages errors.
> 
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> v7: Introduced
> v8: Use errno to report errors
> 
>  tools/libs/ctrl/xc_resource.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tools/libs/ctrl/xc_resource.c b/tools/libs/ctrl/xc_resource.c
> index cb6a97202b..ac1524d1bd 100644
> --- a/tools/libs/ctrl/xc_resource.c
> +++ b/tools/libs/ctrl/xc_resource.c
> @@ -28,7 +28,10 @@ static int xc_resource_op_one(xc_interface *xch, xc_resource_op_t *op)
>                                  XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>  
>      if ( xc_hypercall_bounce_pre(xch, entries) )
> +    {
> +        errno = ENOMEM;

Looking at xc_hypercall_bounce_pre(), it's looks like `errno` should
already be set. On Linux, that would be `mmap()` or `madvise()` updating
it.

>          return -1;
> +    }
>  
>      platform_op.cmd = XENPF_resource_op;
>      platform_op.u.resource_op.nr_entries = op->nr_entries;
> @@ -54,11 +57,15 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>      call_list = xc_hypercall_buffer_alloc(xch, call_list,
>                                            sizeof(*call_list) * nr_ops);
>      if ( !call_list )
> +    {
> +        errno = ENOMEM;

Here, xc_hypercall_buffer_alloc() should already have updated `errno`.
(It's a function called by xc_hypercall_bounce_pre(), so we've got the
same culprit updating `errno`.)

>          return -1;
> +    }
>  
>      platform_ops = xc_hypercall_buffer_array_create(xch, nr_ops);
>      if ( !platform_ops )
>      {
> +        errno = ENOMEM;

Here, xc_hypercall_buffer_array_create() calls `calloc()` and `malloc()`
which will update `errno`.

>          rc = -1;
>          goto out;
>      }
> @@ -66,6 +73,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>      entries_list = xc_hypercall_buffer_array_create(xch, nr_ops);
>      if ( !entries_list )
>      {
> +        errno = ENOMEM;

Same as above.

>          rc = -1;
>          goto out;
>      }
> @@ -81,6 +89,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>                          platform_op, sizeof(xen_platform_op_t));
>          if ( !platform_op )
>          {
> +            errno = ENOMEM;

With xc_hypercall_buffer_array_alloc(), `errno` is updated by `mmap` or
`madvise`, like the first case.

>              rc = -1;
>              goto out;
>          }
> @@ -90,6 +99,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>                     entries, entries_size);
>          if ( !entries)
>          {
> +            errno = ENOMEM;

Same as above.

>              rc = -1;
>              goto out;
>          }
> @@ -137,6 +147,7 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops)
>      if ( nr_ops > 1 )
>          return xc_resource_op_multi(xch, nr_ops, ops);
>  
> +    errno = EINVAL;

Ha! This one was missing indeed.

The patch description will need to be updated with all the chunk be one
been dropped.

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v8 1/2] libxc: Report consistent errors in xc_resource_op

[Teddy Astie]

Le 02/03/2026 à 16:14, Anthony PERARD a écrit :
> On Fri, Feb 27, 2026 at 05:00:05PM +0000, Teddy Astie wrote:
>> xc_report_op doesn't update errno in some error conditions.
>> Make sure it reports -ENOMEM in out of memory errors and -EINVAL
>> in invalid usages errors.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> v7: Introduced
>> v8: Use errno to report errors
>>
>>   tools/libs/ctrl/xc_resource.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/tools/libs/ctrl/xc_resource.c b/tools/libs/ctrl/xc_resource.c
>> index cb6a97202b..ac1524d1bd 100644
>> --- a/tools/libs/ctrl/xc_resource.c
>> +++ b/tools/libs/ctrl/xc_resource.c
>> @@ -28,7 +28,10 @@ static int xc_resource_op_one(xc_interface *xch, xc_resource_op_t *op)
>>                                   XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>>
>>       if ( xc_hypercall_bounce_pre(xch, entries) )
>> +    {
>> +        errno = ENOMEM;
>
> Looking at xc_hypercall_bounce_pre(), it's looks like `errno` should
> already be set. On Linux, that would be `mmap()` or `madvise()` updating
> it.
>
>>           return -1;
>> +    }
>>
>>       platform_op.cmd = XENPF_resource_op;
>>       platform_op.u.resource_op.nr_entries = op->nr_entries;
>> @@ -54,11 +57,15 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>>       call_list = xc_hypercall_buffer_alloc(xch, call_list,
>>                                             sizeof(*call_list) * nr_ops);
>>       if ( !call_list )
>> +    {
>> +        errno = ENOMEM;
>
> Here, xc_hypercall_buffer_alloc() should already have updated `errno`.
> (It's a function called by xc_hypercall_bounce_pre(), so we've got the
> same culprit updating `errno`.)
>
>>           return -1;
>> +    }
>>
>>       platform_ops = xc_hypercall_buffer_array_create(xch, nr_ops);
>>       if ( !platform_ops )
>>       {
>> +        errno = ENOMEM;
>
> Here, xc_hypercall_buffer_array_create() calls `calloc()` and `malloc()`
> which will update `errno`.
>
>>           rc = -1;
>>           goto out;
>>       }
>> @@ -66,6 +73,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>>       entries_list = xc_hypercall_buffer_array_create(xch, nr_ops);
>>       if ( !entries_list )
>>       {
>> +        errno = ENOMEM;
>
> Same as above.
>
>>           rc = -1;
>>           goto out;
>>       }
>> @@ -81,6 +89,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>>                           platform_op, sizeof(xen_platform_op_t));
>>           if ( !platform_op )
>>           {
>> +            errno = ENOMEM;
>
> With xc_hypercall_buffer_array_alloc(), `errno` is updated by `mmap` or
> `madvise`, like the first case.
>
>>               rc = -1;
>>               goto out;
>>           }
>> @@ -90,6 +99,7 @@ static int xc_resource_op_multi(xc_interface *xch, uint32_t nr_ops, xc_resource_
>>                      entries, entries_size);
>>           if ( !entries)
>>           {
>> +            errno = ENOMEM;
>
> Same as above.
>

Ok for all, though good to note that malloc ones are documented as
platform specific behaviors (at least in malloc(3)).
But in our case (POSIX platforms), it's supposed to be the case.

>>               rc = -1;
>>               goto out;
>>           }
>> @@ -137,6 +147,7 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops)
>>       if ( nr_ops > 1 )
>>           return xc_resource_op_multi(xch, nr_ops, ops);
>>
>> +    errno = EINVAL;
>
> Ha! This one was missing indeed.
>
> The patch description will need to be updated with all the chunk be one
> been dropped.
>

Overall, it's

libxc: Report EINVAL on incorrect xc_resource_op usage

When passing 0 operations to xc_resource_op, a error is reported without
setting errno appropriately.

> 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