1. Patchew
  2. Xen
  3. View series

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

Jan Beulich posted 1 patch 2 years, 4 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/ac9ffddc-b102-9876-7a46-345078c3423c@suse.com
[PATCH v2] libxc: avoid clobbering errno in xc_domain_pod_target()
Posted by Jan Beulich 2 years, 4 months ago 
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()
Posted by Juergen Gross 2 years, 4 months ago 
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
Re: [PATCH v2] libxc: avoid clobbering errno in xc_domain_pod_target()
Posted by Bertrand Marquis 2 years, 4 months ago 
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()
Posted by Jan Beulich 2 years, 4 months ago 
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()
Posted by Bertrand Marquis 2 years, 4 months ago 
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()
Posted by Jan Beulich 2 years, 4 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.