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

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

[Jan Beulich]

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). There's
also no reason to overwrite "err".

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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, and hence its setting to zero may also be
worthwhile to drop.
---
v2: Don't save/restore errno, as DPRINTF() already does so.

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

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 340 bytes --]

On 10.12.21 14:11, 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). There's
> also no reason to overwrite "err".
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Bertrand Marquis]

Hi Jan

> On 10 Dec 2021, at 13:11, Jan Beulich <jbeulich@suse.com> 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). There's
> also no reason to overwrite "err".
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

But if err can really only be 0 or -1, I do wonder if the else forcing err to 0 could
be removed but I must say I have no idea if do_memory_op could return a value >0.

Anyway not related to the patch itself.

Cheers
Bertrand

> ---
> 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, and hence its setting to zero may also be
> worthwhile to drop.
> ---
> v2: Don't save/restore errno, as DPRINTF() already does so.
> 
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -1230,13 +1230,9 @@ static int xc_domain_pod_target(xc_inter
>     err = do_memory_op(xch, op, &pod_target, sizeof(pod_target));
> 
>     if ( err < 0 )
> -    {
>         DPRINTF("Failed %s_pod_target dom %d\n",
>                 (op==XENMEM_set_pod_target)?"set":"get",
>                 domid);
> -        errno = -err;
> -        err = -1;
> -    }
>     else
>         err = 0;
> 
> 
> 

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

[Jan Beulich]

On 10.12.2021 14:50, Bertrand Marquis wrote:
>> On 10 Dec 2021, at 13:11, Jan Beulich <jbeulich@suse.com> 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). There's
>> also no reason to overwrite "err".
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks.

> But if err can really only be 0 or -1, I do wonder if the else forcing err to 0 could
> be removed but I must say I have no idea if do_memory_op could return a value >0.

Indeed - see ...

>> ---
>> 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, and hence its setting to zero may also be
>> worthwhile to drop.
>> ---

... this.

Jan

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

[Bertrand Marquis]

Hi Jan,

> On 10 Dec 2021, at 13:54, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 10.12.2021 14:50, Bertrand Marquis wrote:
>>> On 10 Dec 2021, at 13:11, Jan Beulich <jbeulich@suse.com> 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). There's
>>> also no reason to overwrite "err".
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Thanks.
> 
>> But if err can really only be 0 or -1, I do wonder if the else forcing err to 0 could
>> be removed but I must say I have no idea if do_memory_op could return a value >0.
> 
> Indeed - see ...
> 
>>> ---
>>> 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, and hence its setting to zero may also be
>>> worthwhile to drop.
>>> ---
> 
> ... this.

So the else should be dropped then, why not doing it and just mentioning it there ?

Bertrand

> 
> Jan
> 

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

[Jan Beulich]

On 10.12.2021 15:00, Bertrand Marquis wrote:
>> On 10 Dec 2021, at 13:54, Jan Beulich <jbeulich@suse.com> wrote:
>> On 10.12.2021 14:50, Bertrand Marquis wrote:
>>>> On 10 Dec 2021, at 13:11, Jan Beulich <jbeulich@suse.com> 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). There's
>>>> also no reason to overwrite "err".
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Thanks.
>>
>>> But if err can really only be 0 or -1, I do wonder if the else forcing err to 0 could
>>> be removed but I must say I have no idea if do_memory_op could return a value >0.
>>
>> Indeed - see ...
>>
>>>> ---
>>>> 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, and hence its setting to zero may also be
>>>> worthwhile to drop.
>>>> ---
>>
>> ... this.
> 
> So the else should be dropped then, why not doing it and just mentioning it there ?

Well, I'd like confirmation by a maintainer. There are a few aspects to how
things are done in the tool stack which I'm not always aware of. IOW there
might be reasons to keep things as they are after this variant of the patch.

Jan