| Message ID | 20230613162220.3052184-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/evtchn: Purge ERROR_EXIT{,_DOM}() | expand |
Hi, On 13/06/2023 17:22, Andrew Cooper wrote: > These are disliked specifically by MISRA, but they also interfere with code Please explicitly name the rule. > legibility by hiding control flow. Expand and drop them. > > * Rearrange the order of actions to write into rc, then render rc in the > gdprintk(). > * Drop redundant "rc = rc" assignments > * Switch to using %pd for rendering domains > > No functional change. Resulting binary is identical. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Julien Grall <jgrall@amazon.com> [...] > base-commit: f29363922c1b41310c3d87fd9a861ffa9db9204a I notice you have a few e-mail contain this tag. This is a pretty useful when reviewing patches. Do you know which tool is creating it? Cheers,
On 13/06/2023 6:39 pm, Julien Grall wrote: > Hi, > > On 13/06/2023 17:22, Andrew Cooper wrote: >> These are disliked specifically by MISRA, but they also interfere >> with code > > Please explicitly name the rule. I can't remember it off the top of my head. Stefano/Bertrand? > >> legibility by hiding control flow. Expand and drop them. >> >> * Rearrange the order of actions to write into rc, then render rc >> in the >> gdprintk(). >> * Drop redundant "rc = rc" assignments >> * Switch to using %pd for rendering domains >> >> No functional change. Resulting binary is identical. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> Thanks. > >> base-commit: f29363922c1b41310c3d87fd9a861ffa9db9204a > > I notice you have a few e-mail contain this tag. This is a pretty > useful when reviewing patches. Do you know which tool is creating it? Plain git, and the capability has been around for years and years but nigh on impossible to search for or find in the manual. Searching for "base-commit" gets you a tonne of intros of how to rebase. You want [format] useAutoBase = "whenAble" or pass --base=auto to git-format-patch ~Andrew
On 13/06/23 19:45, Andrew Cooper wrote: > On 13/06/2023 6:39 pm, Julien Grall wrote: >> Hi, >> >> On 13/06/2023 17:22, Andrew Cooper wrote: >>> These are disliked specifically by MISRA, but they also interfere >>> with code >> >> Please explicitly name the rule. > > I can't remember it off the top of my head. > > Stefano/Bertrand? Rule 2.1 >>> legibility by hiding control flow. Expand and drop them. >>> >>> * Rearrange the order of actions to write into rc, then render rc >>> in the >>> gdprintk(). >>> * Drop redundant "rc = rc" assignments >>> * Switch to using %pd for rendering domains >>> >>> No functional change. Resulting binary is identical. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> Reviewed-by: Julien Grall <jgrall@amazon.com> > > Thanks. > >> >>> base-commit: f29363922c1b41310c3d87fd9a861ffa9db9204a >> >> I notice you have a few e-mail contain this tag. This is a pretty >> useful when reviewing patches. Do you know which tool is creating it? > > Plain git, and the capability has been around for years and years but > nigh on impossible to search for or find in the manual. Searching for > "base-commit" gets you a tonne of intros of how to rebase. > > You want > > [format] > useAutoBase = "whenAble" > > or pass --base=auto to git-format-patch > > ~Andrew >
On 13.06.2023 21:47, Roberto Bagnara wrote: > On 13/06/23 19:45, Andrew Cooper wrote: >> On 13/06/2023 6:39 pm, Julien Grall wrote: >>> Hi, >>> >>> On 13/06/2023 17:22, Andrew Cooper wrote: >>>> These are disliked specifically by MISRA, but they also interfere >>>> with code >>> >>> Please explicitly name the rule. >> >> I can't remember it off the top of my head. >> >> Stefano/Bertrand? > > Rule 2.1 That's about unreachable code, but inside the constructs there's nothing that's unreachable afaics. Plus expanding "manually" them wouldn't change reachability, would it? Jan
On 13.06.2023 18:22, Andrew Cooper wrote: > These are disliked specifically by MISRA, but they also interfere with code > legibility by hiding control flow. Expand and drop them. > > * Rearrange the order of actions to write into rc, then render rc in the > gdprintk(). > * Drop redundant "rc = rc" assignments > * Switch to using %pd for rendering domains With this change, ... > No functional change. Resulting binary is identical. ... I doubt this. Even .text being entirely identical would be pure luck, as message offsets might change slightly depending on how much padding the compiler inserts between them. Furthermore I wonder whether ... > @@ -336,7 +319,11 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) > > port = rc = evtchn_get_port(d, port); > if ( rc < 0 ) > - ERROR_EXIT(rc); > + { > + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); > + goto out; > + } ... it wouldn't make sense to mention the actual operation that failed, now that each function has its own message(s). In turn I question the usesfulness of "error" in the message text. Then again I wonder whether it isn't time to purge these gdprintk()s altogether. Surely they served a purpose for bringing up initial Linux and mini-os and alike, but that's been two decades ago now. Jan
Hi Jan, On 14/06/2023 07:59, Jan Beulich wrote: > On 13.06.2023 18:22, Andrew Cooper wrote: >> These are disliked specifically by MISRA, but they also interfere with code >> legibility by hiding control flow. Expand and drop them. >> >> * Rearrange the order of actions to write into rc, then render rc in the >> gdprintk(). >> * Drop redundant "rc = rc" assignments >> * Switch to using %pd for rendering domains > > With this change, ... > >> No functional change. Resulting binary is identical. > > ... I doubt this. Even .text being entirely identical would be pure luck, > as message offsets might change slightly depending on how much padding > the compiler inserts between them. Furthermore I wonder whether ... > >> @@ -336,7 +319,11 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) >> >> port = rc = evtchn_get_port(d, port); >> if ( rc < 0 ) >> - ERROR_EXIT(rc); >> + { >> + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); >> + goto out; >> + } > > ... it wouldn't make sense to mention the actual operation that failed, > now that each function has its own message(s). In turn I question the > usesfulness of "error" in the message text. > > Then again I wonder whether it isn't time to purge these gdprintk()s > altogether. Surely they served a purpose for bringing up initial Linux > and mini-os and alike, but that's been two decades ago now. There are still port of new OS on Xen (e.g. QNX, FreeRTOS...) happening time to time. Also, having messages in error path is something I wish most of Xen had. Quite often when I end up to debug an hypercall, I will add printks(). So I am not in favor of removing the gdprintk()s. Cheers,
On 14/06/2023 7:52 am, Jan Beulich wrote: > On 13.06.2023 21:47, Roberto Bagnara wrote: >> On 13/06/23 19:45, Andrew Cooper wrote: >>> On 13/06/2023 6:39 pm, Julien Grall wrote: >>>> Hi, >>>> >>>> On 13/06/2023 17:22, Andrew Cooper wrote: >>>>> These are disliked specifically by MISRA, but they also interfere >>>>> with code >>>> Please explicitly name the rule. >>> I can't remember it off the top of my head. >>> >>> Stefano/Bertrand? >> Rule 2.1 > That's about unreachable code, but inside the constructs there's nothing > that's unreachable afaics. Plus expanding "manually" them wouldn't change > reachability, would it? I bet it's complaining about the while() after the goto. I can see why things end up caring - because this violation can only be spotted in the fully-preprocessed source where the macro-ness has gone away, and *then* applying blanket rules. Which comes back to the original point I made on the call yesterday that do{}while(0) correctness for macros is far more important than some, honestly suspect, claim about the resulting code being somehow "better" without the macro safety. ~Andrew
On 14.06.2023 11:21, Andrew Cooper wrote: > On 14/06/2023 7:52 am, Jan Beulich wrote: >> On 13.06.2023 21:47, Roberto Bagnara wrote: >>> On 13/06/23 19:45, Andrew Cooper wrote: >>>> On 13/06/2023 6:39 pm, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 13/06/2023 17:22, Andrew Cooper wrote: >>>>>> These are disliked specifically by MISRA, but they also interfere >>>>>> with code >>>>> Please explicitly name the rule. >>>> I can't remember it off the top of my head. >>>> >>>> Stefano/Bertrand? >>> Rule 2.1 >> That's about unreachable code, but inside the constructs there's nothing >> that's unreachable afaics. Plus expanding "manually" them wouldn't change >> reachability, would it? > > I bet it's complaining about the while() after the goto. > > I can see why things end up caring - because this violation can only be > spotted in the fully-preprocessed source where the macro-ness has gone > away, and *then* applying blanket rules. Hmm, perhaps. > Which comes back to the original point I made on the call yesterday that > do{}while(0) correctness for macros is far more important than some, > honestly suspect, claim about the resulting code being somehow "better" > without the macro safety. Even further I would claim that the while(0) part of the construct isn't unreachable code simply because it doesn't result in any code being generated. But of course "code" here may mean "source code", not the resulting binary representation. Jan
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index f5e0b12d1520..a7a004a08429 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -36,23 +36,6 @@ #include <asm/guest.h> #endif -#define ERROR_EXIT(_errno) \ - do { \ - gdprintk(XENLOG_WARNING, \ - "EVTCHNOP failure: error %d\n", \ - (_errno)); \ - rc = (_errno); \ - goto out; \ - } while ( 0 ) -#define ERROR_EXIT_DOM(_errno, _dom) \ - do { \ - gdprintk(XENLOG_WARNING, \ - "EVTCHNOP failure: domain %d, error %d\n", \ - (_dom)->domain_id, (_errno)); \ - rc = (_errno); \ - goto out; \ - } while ( 0 ) - #define consumer_is_xen(e) (!!(e)->xen_consumer) /* @@ -336,7 +319,11 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) port = rc = evtchn_get_port(d, port); if ( rc < 0 ) - ERROR_EXIT(rc); + { + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } + rc = 0; chn = evtchn_from_port(d, port); @@ -412,17 +399,30 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, lport = rc = evtchn_get_port(ld, lport); if ( rc < 0 ) - ERROR_EXIT(rc); + { + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } + rc = 0; lchn = evtchn_from_port(ld, lport); rchn = _evtchn_from_port(rd, rport); if ( !rchn ) - ERROR_EXIT_DOM(-EINVAL, rd); + { + rc = -EINVAL; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: %pd, error %d\n", rd, rc); + goto out; + } + if ( (rchn->state != ECS_UNBOUND) || (rchn->u.unbound.remote_domid != ld->domain_id) ) - ERROR_EXIT_DOM(-EINVAL, rd); + { + rc = -EINVAL; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: %pd, error %d\n", rd, rc); + goto out; + } rc = xsm_evtchn_interdomain(XSM_HOOK, ld, lchn, rd, rchn); if ( rc ) @@ -487,11 +487,19 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) write_lock(&d->event_lock); if ( read_atomic(&v->virq_to_evtchn[virq]) ) - ERROR_EXIT(-EEXIST); + { + rc = -EEXIST; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } port = rc = evtchn_get_port(d, port); if ( rc < 0 ) - ERROR_EXIT(rc); + { + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } + rc = 0; chn = evtchn_from_port(d, port); @@ -535,7 +543,11 @@ static int evtchn_bind_ipi(evtchn_bind_ipi_t *bind) write_lock(&d->event_lock); if ( (port = get_free_port(d)) < 0 ) - ERROR_EXIT(port); + { + rc = port; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } chn = evtchn_from_port(d, port); @@ -599,16 +611,29 @@ static int evtchn_bind_pirq(evtchn_bind_pirq_t *bind) write_lock(&d->event_lock); if ( pirq_to_evtchn(d, pirq) != 0 ) - ERROR_EXIT(-EEXIST); + { + rc = -EEXIST; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } if ( (port = get_free_port(d)) < 0 ) - ERROR_EXIT(port); + { + rc = port; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } chn = evtchn_from_port(d, port); info = pirq_get_info(d, pirq); if ( !info ) - ERROR_EXIT(-ENOMEM); + { + rc = -ENOMEM; + gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); + goto out; + } + info->evtchn = port; rc = (!is_hvm_domain(d) ? pirq_guest_bind(v, info,
These are disliked specifically by MISRA, but they also interfere with code legibility by hiding control flow. Expand and drop them. * Rearrange the order of actions to write into rc, then render rc in the gdprintk(). * Drop redundant "rc = rc" assignments * Switch to using %pd for rendering domains No functional change. Resulting binary is identical. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> --- xen/common/event_channel.c | 79 +++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 27 deletions(-) base-commit: f29363922c1b41310c3d87fd9a861ffa9db9204a