| Message ID | 912e2574-2adf-25cf-498f-86a86f31c30d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxc: avoid clobbering errno in xc_domain_pod_target() | expand |
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
--- 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
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.