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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

~Andrew

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