Re: [Xen-devel] [PATCH] IOMMU: replace ASSERT()s checking for NULL

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.11.16 at 11:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/11/16 09:53, Jan Beulich wrote:
>>>>> On 07.11.16 at 10:24, <JBeulich@xxxxxxxx> wrote:
>>> --- a/xen/drivers/passthrough/io.c
>>> +++ b/xen/drivers/passthrough/io.c
>>> @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data)
>>>      spin_lock(&irq_map->dom->event_lock);
>>>  
>>>      dpci = domain_get_irq_dpci(irq_map->dom);
>>> -    ASSERT(dpci);
>>> +    if ( unlikely(!dpci) )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        return;
>>> +    }
>>>      list_for_each_entry ( digl, &irq_map->digl_list, list )
>>>      {
>>>          unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, 
>>> digl->intx);
>>> @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d,
>>>  
>>>  static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci 
>>> *pirq_dpci)
>>>  {
>>> -    ASSERT(d->arch.hvm_domain.irq.dpci);
>>> +    if ( unlikely(!d->arch.hvm_domain.irq.dpci) )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        return;
>>> +    }
>> I wonder, btw, whether we shouldn't ease these by making a macro
>> along the lines of
>>
>> #define ASSERT_BAIL(cond, retval...) do { \
>>     if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \
>> } while (0)
>>
>> Opinions?
> 
> On the one hand, this is becoming a common pattern.  On the other, I
> really dislike hiding control flow in a macro like this.
> 
> It might be ok if named ASSERT_UNREACHABLE_RETURN() to both highlight
> that it is an ASSERT_UNREACHABLE() rather than an ASSERT() of the
> condition passed.  Perhaps  ASSERT_UNREACHABLE_RETURN_IF() to avoid
> mixing up the types of assertion?

That would end up being (taking one of the examples above)

static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
    ASSERT_UNREACHABLE_RETURN_IF(d->arch.hvm_domain.irq.dpci);
    ...

or

static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
    ASSERT_UNREACHABLE_RETURN_IF(!d->arch.hvm_domain.irq.dpci);
    ...

No matter which way I take it, I find it confusing: Is the condition
meant to be ASSERT()-like or if()-like? If hiding control flow keywords
in a macro looks bad to you, how about

#define ASSERT_BAIL(cond, stmt...) do { \
    if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); stmt; } \
} while (0)

i.e. forcing the "return" to be visible at the macro invocation site
(and at once allowing e.g. using "break" or "goto" there too)? It
might even be worthwhile calling it ASSERT_CLEANUP() or some
such, making it more logical to use even non-control-flow
statements there (like setting a variable to a certain value).

Jan


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