Skip site navigation (1)Skip section navigation (2) 
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-all@freebsd.org  Thu Jan 25 05:15:45 2018
Return-Path: <owner-svn-src-all@freebsd.org>
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 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-all@freebsd.org
X-Mailman-Version: 2.1.25
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for &quot;
 user&quot; and &quot; projects&quot; \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-all>,
 <mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>;
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-all>,
 <mailto:svn-src-all-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>