Re: [XEN PATCH] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2024 10:02, Federico Serafini wrote:
> On 11/03/24 08:40, Jan Beulich wrote:
>> On 08.03.2024 12:51, Federico Serafini wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>>>   
>>>       case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>>>           return arch_virq_is_global(virq);
>>> +
>>> +    default:
>>> +        ASSERT(virq < NR_VIRQS);
>>> +        break;
>>>       }
>>>   
>>> -    ASSERT(virq < NR_VIRQS);
>>>       return true;
>>>   }
>>
>> Just for my understanding: The ASSERT() is moved so the "default" would
>> consist of more than just "break". Why is it that then the "return" isn't
>> moved, too?
> 
> No reason in particular.
> If preferred, I can move it too.

I for one would prefer that, yes. But what's needed up front is that we
decide what we want to do _consistently_ in all such cases.

>>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>>           case ECS_VIRQ:
>>>               printk(" v=%d", chn->u.virq);
>>>               break;
>>> +        default:
>>> +            /* Nothing to do in other cases. */
>>> +            break;
>>>           }
>>
>> Yes this, just to mention it, while in line with what Misra demands is
>> pretty meaningless imo: The absence of "default" says exactly what the
>> comment now says. FTAOD - this is a comment towards the Misra guideline,
>> not so much towards the specific change here.
> 
> Both you and Stefano reviewed the code and agreed on the fact that doing
> nothing for the default case is the right thing and now the code
> explicitly says that without letting any doubts.
> Furthermore, during the reviews it could happen that you notice a switch
> where something needs to be done for the default case.

That shouldn't happen during review. Anyone proposing a patch to add such
a comment wants to first have made sure the comment is actually applicable
there. Otherwise we're in "mechanically add comments" territory, which I
think we all agreed we want to avoid.

Jan