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

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

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.

> > @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport)
> >           break;
> >       default:
> >           ret = -EINVAL;
> > +        break;
> >       }
> I certainly agree here.
>
> > @@ -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.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)