do_memory_op() supplies return value and has errno set the usual way.
Don't overwrite errno with 1 (aka EPERM on at least Linux).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
An alternative would be to let go of the DPRINTK() and leave errno and
err alone altogether. While the hypervisor side of the hypercall gives
the impression of being able to return positive values as of
637a283f17eb ("PoD: Allow pod_set_cache_target hypercall to be
preempted"), due to the use of "rc >= 0" there, afaict that's not
actually the case. IOW "err" can really only be 0 or -1 here.

--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1231,10 +1231,11 @@ static int xc_domain_pod_target(xc_inter
 
     if ( err < 0 )
     {
+        err = errno;
         DPRINTF("Failed %s_pod_target dom %d\n",
                 (op==XENMEM_set_pod_target)?"set":"get",
                 domid);
-        errno = -err;
+        errno = err;
         err = -1;
     }
     else

On 09.12.21 11:26, Jan Beulich wrote:
> do_memory_op() supplies return value and has errno set the usual way.
> Don't overwrite errno with 1 (aka EPERM on at least Linux).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> An alternative would be to let go of the DPRINTK() and leave errno and
> err alone altogether. While the hypervisor side of the hypercall gives
> the impression of being able to return positive values as of
> 637a283f17eb ("PoD: Allow pod_set_cache_target hypercall to be
> preempted"), due to the use of "rc >= 0" there, afaict that's not
> actually the case. IOW "err" can really only be 0 or -1 here.
> 
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -1231,10 +1231,11 @@ static int xc_domain_pod_target(xc_inter
>   
>       if ( err < 0 )
>       {
> +        err = errno;
>           DPRINTF("Failed %s_pod_target dom %d\n",
>                   (op==XENMEM_set_pod_target)?"set":"get",
>                   domid);
> -        errno = -err;
> +        errno = err;

DPRINTF() won't change errno, so I think you should just drop the line
overwriting errno.


Juergen

On 09.12.21 11:36, Juergen Gross wrote:
> On 09.12.21 11:26, Jan Beulich wrote:
>> do_memory_op() supplies return value and has errno set the usual way.
>> Don't overwrite errno with 1 (aka EPERM on at least Linux).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> An alternative would be to let go of the DPRINTK() and leave errno and
>> err alone altogether. While the hypervisor side of the hypercall gives
>> the impression of being able to return positive values as of
>> 637a283f17eb ("PoD: Allow pod_set_cache_target hypercall to be
>> preempted"), due to the use of "rc >= 0" there, afaict that's not
>> actually the case. IOW "err" can really only be 0 or -1 here.
>>
>> --- a/tools/libs/ctrl/xc_domain.c
>> +++ b/tools/libs/ctrl/xc_domain.c
>> @@ -1231,10 +1231,11 @@ static int xc_domain_pod_target(xc_inter
>>       if ( err < 0 )
>>       {
>> +        err = errno;
>>           DPRINTF("Failed %s_pod_target dom %d\n",
>>                   (op==XENMEM_set_pod_target)?"set":"get",
>>                   domid);
>> -        errno = -err;
>> +        errno = err;
> 
> DPRINTF() won't change errno, so I think you should just drop the line
> overwriting errno.

In fact you added the 3rd layer of errno saving here. :-)

DPRINTF() and friends will save errno, and the underlying
xc_report*() functions will do so, too.

Writing a patch to remove the saving at the macro layer...


Juergen

On 09.12.2021 11:41, Juergen Gross wrote:
> On 09.12.21 11:36, Juergen Gross wrote:
>> On 09.12.21 11:26, Jan Beulich wrote:
>>> do_memory_op() supplies return value and has errno set the usual way.
>>> Don't overwrite errno with 1 (aka EPERM on at least Linux).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> An alternative would be to let go of the DPRINTK() and leave errno and
>>> err alone altogether. While the hypervisor side of the hypercall gives
>>> the impression of being able to return positive values as of
>>> 637a283f17eb ("PoD: Allow pod_set_cache_target hypercall to be
>>> preempted"), due to the use of "rc >= 0" there, afaict that's not
>>> actually the case. IOW "err" can really only be 0 or -1 here.
>>>
>>> --- a/tools/libs/ctrl/xc_domain.c
>>> +++ b/tools/libs/ctrl/xc_domain.c
>>> @@ -1231,10 +1231,11 @@ static int xc_domain_pod_target(xc_inter
>>>       if ( err < 0 )
>>>       {
>>> +        err = errno;
>>>           DPRINTF("Failed %s_pod_target dom %d\n",
>>>                   (op==XENMEM_set_pod_target)?"set":"get",
>>>                   domid);
>>> -        errno = -err;
>>> +        errno = err;
>>
>> DPRINTF() won't change errno, so I think you should just drop the line
>> overwriting errno.
> 
> In fact you added the 3rd layer of errno saving here. :-)
> 
> DPRINTF() and friends will save errno, and the underlying
> xc_report*() functions will do so, too.

I guess I should have checked ... Question is then whether setting
"err" to 0 on the "else" path could/should then also be dropped (its
setting to -1 clearly can be, and I already have it that way for v2).

Jan