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

[Julien Grall <julien.grall@linaro.org>]

On 13/01/15 17:18, Ian Campbell wrote:
> 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.

I spotted more. Anyway, I will move to a if (!do_smth()) BUG() form.

Regards,

-- 
Julien Grall