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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 22 Jun 2024, Julien Grall wrote:
> 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.

Yes, good idea, we can discuss tomorrow. I'll write down my thinking
about 16.4 here in the meantime to address your question and hopefully
we can align on the approach to take tomorrow.

As you might remember I supported 16.4 and I was keen on having it in
rules.rst because I believe that this rules makes the code better.

16.4 is about ensuring that every switch that is supposed to handle all
possible parameters, actually handle all possible parameters. It is also
about having a default label so that we can handle unexpected parameters
especially in case of integers parameters. (In case of enums we can
check at compile time that we handle all possible parameters even
without a default label.)

In Xen, some switches are expected to handle all possible parameters,
but some of them are not. Specifically the "notifier pattern" switches
are by design not expected to handle all possible parameters. Of course
it can be that some of them have to, but the design of the "notifier
pattern" is that you can handle only the subset you care about, and you
don't need to care about all of the possible parameters.

ECLAIR or gcc cannot help us there. Most of these instances don't want
to handle all parameters. We have to remove the violations from the
scan, either by deviation or by adding a default label. Otherwise the
notifier pattern stops us from making more progress.

There are lots of other switches that are not part of the notifier
pattern and are required to handle all possible parameters. I would like
to make sure we enable the checks for these other switches where ECLAIR
and gcc can actually help immediately.

I do realize that some of the notifier pattern switches might want to
handle all parameters but Bugseng or anyone else looking for simple
improvements are not in the position to tell which ones they are. We
need to wait for a maintainer or expert in the specific code to spot
them. It is not a good idea to cause a delay in handling all the
remaining 16.4 more interesting switches (which is also easy) to better
handle the notifier pattern (which is hard).

The notifier pattern can be looked at separately later by the relevant
maintainer / interested community members by sending case-by-case
improvements. They cannot be mechanically resolved. My understanding is
that with this patch series committed we would be close to zero
violations for 16.4.