Re: [RFC PATCH 1/3] misra: address 5.5 pirq_cleanup_check

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 29 Jul 2025, Andrew Cooper wrote:
> On 29/07/2025 10:24 pm, Dmytro Prokopchuk1 wrote:
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index c8c1bfa615..2efb5f5c78 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -672,7 +672,7 @@ static int evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
> >      if ( rc != 0 )
> >      {
> >          info->evtchn = 0;
> > -        pirq_cleanup_check(info, d);
> > +        PIRQ_CLEANUP_CHECK(info, d);
> 
> Well, this is awkward.  This is dead code, but only when you realise ...
> 
> > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> > index 95034c0d6b..958d0b1aca 100644
> > --- a/xen/include/xen/irq.h
> > +++ b/xen/include/xen/irq.h
> > @@ -185,7 +185,7 @@ extern struct pirq *pirq_get_info(struct domain *d, int 
> > pirq);
> >  
> >  void pirq_cleanup_check(struct pirq *pirq, struct domain *d);
> >  
> > -#define pirq_cleanup_check(pirq, d) \
> > +#define PIRQ_CLEANUP_CHECK(pirq, d) \
> >      (!(pirq)->evtchn ? pirq_cleanup_check(pirq, d) : (void)0)
> >  
> 
> ... what this is really doing.
> 
> Looking at this overall diff, it really is outrageous that we're hiding
> a conditional call like this.
> 
> We should just remove the macro, and expand
> 
>     if ( !pirq->evtchn )
>         pirq_cleanup_check(pirq, d);
> 
> in most of the callsites.  The overall diff will be smaller (no need to
> change the comments), and the end result is proper regular normal C.
 
Yes, would look much better. +1 from me.