Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2015-01-13 at 16:57 +0000, Julien Grall wrote:
> (CC Jan)

I think you forget, I added him.

> >>>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
> >>>>  {
> >>>>      d->arch.phys_timer_base.offset = NOW();
> >>>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> >>>> +
> >>>> +    /* At this stage vgic_reserve_virq can't fail */
> >>>> +    if ( is_hardware_domain(d) )
> >>>> +    {
> >>>> +        BUG_ON(!vgic_reserve_virq(d, 
> >>>> timer_get_irq(TIMER_PHYS_SECURE_PPI)));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, 
> >>>> timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
> >>>> +    }
> >>>> +    else
> >>>> +    {
> >>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
> >>>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> >>>
> >>> Although BUG_ON is not conditional on $debug I think we still should
> >>> avoid side effects in the condition.
> >>
> >> I know, but this should never fail as it called during on domain
> >> construction. If so we may have some other issue later if we decide to
> >> assign PPI to a guest.
> >>
> >> I would prefer to keep the BUG_ON here
> > 
> > I'm not objecting the the BUG_ON itself but to the fact that the
> > condition has a side effect. Please use:
> >         if (!do_something())
> >             BUG()
> > instead to avoid this.
> 
> We have other place in the code where BUG_ON as a side-effect.

If we do then it is a tiny minority of places, and they are IMHO wrong.
I spotted one in the 600+ results of grepping for BUG_ON.

> IHMO, if (!do_something()) BUG() <=> BUG_ON.

No, BUG_ON() is a variant of ASSERT(), with the distinction being that
the former is not only included when debug=y. It is as wrong to have a
side-effect in the BUG_ON as it is to have one in an ASSERT.

> On the latter you know directly why it's failing, on the former you have
> to look at the code.

If it's important/possible to fail then a log message would be
appropriate.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel