Date: Thu, 25 Jan 2018 14:46:14 +1100 From: "Kristof Provost" <kp@FreeBSD.org> To: "Ian Lepore" <ian@freebsd.org> Cc: "Gleb Smirnoff" <glebius@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328313 - head/sys/netpfil/pf Message-ID: <14734657-ABC2-437B-9830-724901066342@FreeBSD.org> In-Reply-To: <8FA39DD7-FD83-49D5-B7FC-3637B42129BE@FreeBSD.org> References: <201801240429.w0O4THIl059440@repo.freebsd.org> <20180125001310.GJ8113@FreeBSD.org> <1516840498.42536.213.camel@freebsd.org> <8FA39DD7-FD83-49D5-B7FC-3637B42129BE@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 25 Jan 2018, at 12:08, Kristof Provost wrote:
> 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.
>
So this:
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 55ae1145835..0dbf1fe7f66 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1623,7 +1623,6 @@ int
pf_unlink_state(struct pf_state *s, u_int flags)
{
struct pf_idhash *ih = &V_pf_idhash[PF_IDHASH(s)];
- int last;
if ((flags & PF_ENTER_LOCKED) == 0)
PF_HASHROW_LOCK(ih);
@@ -1664,8 +1663,9 @@ pf_unlink_state(struct pf_state *s, u_int flags)
PF_HASHROW_UNLOCK(ih);
pf_detach_state(s);
- last = refcount_release(&s->refs);
- KASSERT(last == 0, ("Incorrect state reference count"));
+ /* pf_state_insert() initialises refs to 2, so we can never
release the
+ * last reference here, only in pf_release_state(). */
+ (void)refcount_release(&s->refs);
return (pf_release_state(s));
}
I do assume that (void) will tell Coverity I’m deliberately ignoring
the return value. It’s a fairly common idiom, so I’d expect it to
understand.
Regards,
Kristof
From owner-svn-src-head@freebsd.org Thu Jan 25 05:15:45 2018
Return-Path: <owner-svn-src-head@freebsd.org>
Delivered-To: svn-src-head@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 404E7EBDDE8;
Thu, 25 Jan 2018 05:15:45 +0000 (UTC)
(envelope-from lwhsu@FreeBSD.org)
Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org
[IPv6:2610:1c1:1:606c::19:3])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mxrelay.nyi.freebsd.org",
Issuer "Let's Encrypt Authority X3" (verified OK))
by mx1.freebsd.org (Postfix) with ESMTPS id DD6D96B1A5;
Thu, 25 Jan 2018 05:15:44 +0000 (UTC)
(envelope-from lwhsu@FreeBSD.org)
Received: from repo.freebsd.org (repo.freebsd.org
[IPv6:2610:1c1:1:6068::e6a:0])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client did not present a certificate)
by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id D4CE6167;
Thu, 25 Jan 2018 05:15:44 +0000 (UTC)
(envelope-from lwhsu@FreeBSD.org)
Received: from repo.freebsd.org ([127.0.1.37])
by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id w0P5Fioo008096;
Thu, 25 Jan 2018 05:15:44 GMT (envelope-from lwhsu@FreeBSD.org)
Received: (from lwhsu@localhost)
by repo.freebsd.org (8.15.2/8.15.2/Submit) id w0P5FiLk008095;
Thu, 25 Jan 2018 05:15:44 GMT (envelope-from lwhsu@FreeBSD.org)
Message-Id: <201801250515.w0P5FiLk008095@repo.freebsd.org>
X-Authentication-Warning: repo.freebsd.org: lwhsu set sender to
lwhsu@FreeBSD.org using -f
From: Li-Wen Hsu <lwhsu@FreeBSD.org>
Date: Thu, 25 Jan 2018 05:15:44 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-head@freebsd.org
Subject: svn commit: r328388 - head/share/man/man8
X-SVN-Group: head
X-SVN-Commit-Author: lwhsu
X-SVN-Commit-Paths: head/share/man/man8
X-SVN-Commit-Revision: 328388
X-SVN-Commit-Repository: base
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-head@freebsd.org
X-Mailman-Version: 2.1.25
Precedence: list
List-Id: SVN commit messages for the src tree for head/-current
<svn-src-head.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-head>,
<mailto:svn-src-head-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>;
List-Post: <mailto:svn-src-head@freebsd.org>
List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-head>,
<mailto:svn-src-head-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 25 Jan 2018 05:15:45 -0000
Author: lwhsu (ports committer)
Date: Thu Jan 25 05:15:44 2018
New Revision: 328388
URL: https://svnweb.freebsd.org/changeset/base/328388
Log:
Fix manual page install on non-amd64
Reviewed by: emaste, imp
Differential Revision: https://reviews.freebsd.org/D14038
Modified:
head/share/man/man8/Makefile
Modified: head/share/man/man8/Makefile
==============================================================================
--- head/share/man/man8/Makefile Thu Jan 25 02:52:44 2018 (r328387)
+++ head/share/man/man8/Makefile Thu Jan 25 05:15:44 2018 (r328388)
@@ -29,14 +29,15 @@ MLINKS= \
.if ${MK_NIS} != "no"
MAN+= yp.8
-MLINKS+=uefi.8 efi.8 \
- yp.8 NIS.8 \
+MLINKS+=yp.8 NIS.8 \
yp.8 nis.8 \
yp.8 YP.8
.endif
.if ${MACHINE_CPUARCH} == "amd64"
_uefi.8= uefi.8
+
+MLINKS+=uefi.8 efi.8
.endif
.include <bsd.prog.mk>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?14734657-ABC2-437B-9830-724901066342>