Re: [PATCH] libxc: avoid clobbering errno in xc_domain_pod_target()

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>> ---
>>> 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