svn commit: r232071 - head/sys/vm

[Rodney W. Grimes]

> On Mon, May 27, 2019 at 07:21:24AM -0700, Rodney W. Grimes wrote:
> > I wish to assert again that all changes based on static
> > analysis tools require a formal code review by at minimum:
> > a)  An expert in static analysis tools or a group of people
> >     selected who we consider language experts.
> > b)  An area expert from the area that is being affected
> > 
> > We as a project look bad by not having this minimal
> > set of code reviews in place for things that are coming
> > from a source that is frought with introducing bad
> > change.
> 
> Perhaps I should've just uploaded the log to let anyone interested
> have a look themselves.  I've been only checking the kernel so far,
> but plan to cover userland as well later.

I do not at all mean to discourage what you are doing, it
is good to go over static analysis reports, the problem
is that there are often many false positives, and also
subtle things that need extreme caution when making
repairs.

I just ask that before a change be made that starts from some
static analysis tool that a formal code review occur before
the change is committed.

>     freefall:/home/danfe/pvs-studio-2019-05-27.log.xz

It would be nice if we had a "team" that looked at all
the coverity data, and any other data like what you have
offered up here.  Part of the problem is that few want
to do that work, or those that do want to think it is
low hanging fruit that anyone can do.  I would suggest
the opposite, some times it might be low hanging fruit
but often it is not, it needs expertise or mistakes
are going to happen, and often those mistakes go
un caught for some time.

Recently a change was made based on a coverity report
about not freeing something, some free's got added,
one of our downsteam consumers caught this change on
import and reported it as a multiple free situation,
which was caused by pointer aliasing, something that
most static analyzers are horrible at.  This self induced
bug was in tree for ~2 months and iirc made it in a release.

I know I personally saw the original commit, and it
looked ok to my un trained eyes, only upon the aliasing
being pointed out did my light bulb go off that this
was a mistake to have changed the code in the way it
was changed.

Can I get a hands up for team "Static Blue"?

> ./danfe
-- 
Rod Grimes                                                 rgrimes at freebsd.org