From owner-svn-src-all@freebsd.org Thu Jan 25 01:08:24 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 3CFE0ED15C4; Thu, 25 Jan 2018 01:08:24 +0000 (UTC) (envelope-from srs0=ghjn=eu=freebsd.org=kp@codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA 2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id D4CDE814D8; Thu, 25 Jan 2018 01:08:23 +0000 (UTC) (envelope-from srs0=ghjn=eu=freebsd.org=kp@codepro.be) Received: from [192.168.228.1] (unknown [138.44.250.147]) (Authenticated sender: kp) by venus.codepro.be (Postfix) with ESMTPSA id 205DCBD35; Thu, 25 Jan 2018 02:08:20 +0100 (CET) From: "Kristof Provost" To: "Ian Lepore" Cc: "Gleb Smirnoff" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328313 - head/sys/netpfil/pf Date: Thu, 25 Jan 2018 12:08:17 +1100 X-Mailer: MailMate (2.0BETAr6103) Message-ID: <8FA39DD7-FD83-49D5-B7FC-3637B42129BE@FreeBSD.org> In-Reply-To: <1516840498.42536.213.camel@freebsd.org> References: <201801240429.w0O4THIl059440@repo.freebsd.org> <20180125001310.GJ8113@FreeBSD.org> <1516840498.42536.213.camel@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 01:08:24 -0000 On 25 Jan 2018, at 11:34, Ian Lepore wrote: > On Wed, 2018-01-24 at 16:13 -0800, Gleb Smirnoff wrote: >> (r328313) >> K> @@ -1613,6 +1613,7 @@ int >> K>  pf_unlink_state(struct pf_state *s, u_int flags) >> K>  { >> K>   struct pf_idhash *ih = &V_pf_idhash[PF_IDHASH(s)]; >> K> + int last; >> K>   >> K>   if ((flags & PF_ENTER_LOCKED) == 0) >> K>   PF_HASHROW_LOCK(ih); >> K> @@ -1653,7 +1654,8 @@ pf_unlink_state(struct pf_state *s, u_int >> flags) >> K>   PF_HASHROW_UNLOCK(ih); >> K>   >> K>   pf_detach_state(s); >> K> - refcount_release(&s->refs); >> K> + last = refcount_release(&s->refs); >> K> + KASSERT(last == 0, ("Incorrect state reference count")); >> K>   >> K>   return (pf_release_state(s)); >> K>  } >> >> IMHO, we shouldn't emit extra code to please Coverity. We can mark it >> as a false positive in the interface. It may make sense to add a >> comment >> for a human to explain why return isn't checked here. >> > > Not to mention that when KASSERT compiles to nothing, what you're left > with is a "defined but not used" warning for 'last'. > I’d really like to keep the KASSERT(), because this is the sort of thing that could go wrong, and the assertion would be helpful. I suppose I could wrap last in #ifdef INVARIANTS, but that’s rather ugly too. Asserting that the refcount is at least 1 when entering pf_release_state() would express the same, but that’s also problematic. Of course, errors should trigger the KASSERT() in refcount_release(), so I think I may have convinced myself that the KASSERT() can in fact be removed and replaced with (void)refcount_release() and a comment explaining why this refcount_release() can never return 1. Regards, Kristof