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.
