Re: [XEN PATCH v2] xen: add explicit comment to identify notifier patterns

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 21/06/2024 23:34, Stefano Stabellini wrote:
> > > > Yes, I also think this could be an opportunity to check the pattern
> > > > but no one has yet been identified to do this.
> > > I don't think I understand Julien's question and/or your answer.
> > >
> > > Is the question whether someone has done an analysis to make sure this
> > > patch covers all notifier patters in the xen codebase?
> > I think Jan and Julien's concerns are about the fact that my patch
> > takes for granted that all the switch statements are doing the right
> > thing: someone should investigate the notifier patterns to confirm that
> > their are handling the different cases correctly.
> That's really difficult to do, even for the maintainers of the code in
> question.
Sure it will require some work, but as any violation. However, I thought 
the whole point of MISRA is to improve the safety and our code base in 
general?
AFAIU, we already have some doubt that some notifiers are correct. So to 
me it seems wrong to add a comment because while this silence MISRA, 
this doesn't solve it in the true spirit.
> And by not taking this patch we are exposing ourselves to more safety
> risks because we cannot make R16.4 blocking.
>
>
> > > If so, I expect that you have done an analysis simply by basing this
> > > patch on the 16.4 violations reported by ECLAIR?
> > The previous version of the patch was based only on the reports of
> > ECLAIR but Jan said "you left out some patterns, why?".
> >
> > So, this version of the patch adds the comment for all the notifier
> > patterns I found using git grep "struct notifier_block \*"
> > (a superset of the ones reported by ECLAIR because some of them are in
> > files excluded from the analysis or deviated).
> I think this patch is a step in the right direction. It doesn't prevent
> anyone in the community from making expert evaluations on whether the
> pattern is implemented correctly.
I am not sure I am reading this correctly. To me you are saying that for 
your MISRA, adding the default case is fine even if we believe some 
notifiers are incorrect. Did I understand right?
If so it does seems odd because this is really not solving the 
violation. You are just putting a smoke screen in front in the hope that 
there are no big issue in the code...
> Honestly, I don't see another way to make progress on this, except for
> maybe deviating project-wide "struct notifier_block". But that's
> conceptually the same thing as this patch.
I still don't quite understand why you are so eager to hide violation 
just that you can progress faster on other bits.
I personally cannot put my name on a patch where the goal is to add a 
comment that no-one verified that it was actually true. (i.e. all the 
cases we care are handled). In particular in the Arm code, because IIRC 
we already identified issues in the past in the notifier.
I think it wouild be worth discussing the approach again in the next 
MISRA meeting.
Cheers,

> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
--
Julien Grall