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 10:51, Jan Beulich wrote:
> 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.
The only reason I left the return at the end is because it is
slightly more compliant to advisory (and not accepted) Rule 15.5 that
says to place a single return at the end of the function and to not use
early returns.

> > > > @@ -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.
What I wanted to say is that adding the comment is not completely
meaningless: it makes the intention of the code explicit and such
intention is double/triple checked.

--
Federico Serafini, M.Sc.

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