[PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Andrew Cooper]

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(-)

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,

base-commit: f29363922c1b41310c3d87fd9a861ffa9db9204a
-- 
2.30.2

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Andrew Cooper]

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

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Roberto Bagnara]

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
> 

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Jan Beulich]

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

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Jan Beulich]

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

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Andrew Cooper]

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

Re: [PATCH] xen/evtchn: Purge ERROR_EXIT{,_DOM}()

[Jan Beulich]

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