Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Feb 2023, Luca Fancellu wrote:
> > On 16 Feb 2023, at 08:19, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > On 16.02.2023 00:49, Stefano Stabellini wrote:
> >> On Wed, 15 Feb 2023, Julien Grall wrote:
> >>> On 14/02/2023 22:25, Stefano Stabellini wrote:
> >>>>> Patch 1's example has a "comment" field, which no entry makes use of.
> >>>>> Without that, how does it become clear _why_ a particular file is to
> >>>>> be excluded? The patch description here also doesn't provide any
> >>>>> justification ...
> >>>> 
> >>>> It would be good to have a couple of pre-canned justifications as the
> >>>> reason for excluding one file could be different from the reason for
> >>>> excluding another file. Some of the reasons:
> >>> 
> >>> I think the reasons should be ambiguous. This is ...
> >>> 
> >>>> - imported from Linux
> >>> 
> >>> ... the case here but...
> >>> 
> >>> This reason is pretty clear to me but...
> >>> 
> >>>> - too many false positives
> >>> 
> >>> ... not here. What is too many?
> >>> 
> >>>> That said, we don't necessarily need to know the exact reason for
> >>>> excluding one file to be able to start scanning xen with cppcheck
> >>>> automatically. If someone wants to remove a file from the exclude list
> >>>> in the future they just need to show that cppcheck does a good job at
> >>>> scanning the file and we can handle the number of violations.
> >>> 
> >>> I disagree. A good reasoning from the start will be helpful to decide 
> >>> when we
> >>> can remove a file from the list. Furthermore, we need to set good example 
> >>> for
> >>> any new file we want to exclude.
> >>> 
> >>> Furthermore, if we exclude some files, then it will be difficult for the
> >>> reviewers to know when they can be removed from the list. What if this is 
> >>> fine
> >>> with CPPCheck but not EClair (or any other)?
> >> 
> >> Yes, the reason would help. In previous incarnations of this work, there
> >> was a request for detailed information on external files, such as:
> >> - where this file is coming from
> >> - if coming from Linux, which version of Linux
> >> - maintenance status
> >> - coding style
> >> 
> >> But this is not what you are asking. You are only asking for a reason
> >> and "imported from Linux" would be good enough. Please correct me if I
> >> am wrong.
> > 
> > I guess you mean s/would/could/. Personally I find "imported from Linux"
> > as an entirely unacceptable justification: Why would the origin of a file
> > matter on whether it has violations? Dealing with the violations may be
> > more cumbersome (because preferably the adjustments would go to the
> > original files first). Yet not dealing with them - especially if there
> > are many - reduces the benefit of the work we do quite a bit, because it
> > may leave much more work for downstreams to do to actually be able to do
> > any certification. That may go to the extent of questioning why we would
> > bother dealing with a few dozen violations if hundreds remain but are
> > hidden.

Yes, we need to figure out a way to deal with these files. However, at
the moment they are getting in the way of easier low hanging fruits.

One "easy" low hanging fruit is to use cppcheck to scan new patches for
new MISRA violations. That would give immediate benefits to the
community. It is not easy to "diff" results with cppcheck and in any
case it would help a lot if we had a cleaner baseline because it would
make it far easier to read the results and make sense of them.

The goal of this exclusion list is to give us that: a cleaner baseline
so that we can make progress faster on low hanging fruits. This list is
not meant to be fixed and stay unchanged for a long time. In fact, it
could even live under automation/ as part of the gitlab-ci test that
triggers cppcheck, if we prefer.


> Hi Jan,
> 
> my personal opinion is that we can’t handle them for files that needs to be 
> kept
> in sync from an external source, because we can’t justify findings or tag 
> false
> positives, if we are lucky we might fix the non compliances but even in that 
> case
> there might be times when the fix goes through and cases where the fix is 
> objected
> by the maintainers just because their project is not following the MISRA 
> standard.
> 
> On our side, what we can do is track these files and from time to time check 
> that
> they are still eligible to backport, because when they are not anymore we 
> could
> just port them to Xen coding style and start doing direct changes.
> 
> 
> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday 
> morning
> I’ve also had a look on the Michal’s file list and I’ve tracked down the 
> origin, here
> my findings, maybe we could merge your list with mine?

Yes to merge the two lists, but I think we should keep only items that we
actually need for the sake of having a cleaner baseline. So I would only
add things we need today to reduce the noise on cppcheck results
(excluding 20.7 and also excluding unusedStructMember which I think we
should probably stop scanning with cppcheck altogether).