Date: Mon, 27 May 2019 14:34:09 -0700 (PDT) From: "Rodney W. Grimes" <freebsd@gndrsh.dnsmgr.net> To: Alexey Dokuchaev <danfe@freebsd.org> Cc: rgrimes@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r232071 - head/sys/vm Message-ID: <201905272134.x4RLY9oO012473@gndrsh.dnsmgr.net> In-Reply-To: <20190527164056.GA17917@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201905272134.x4RLY9oO012473>