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

[Julien Grall <julien@xxxxxxx>]

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