From owner-svn-src-head@freebsd.org Thu Jan 25 01:33:37 2018 Return-Path: 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 5898CED34A5 for ; Thu, 25 Jan 2018 01:33:37 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C1D698270F for ; Thu, 25 Jan 2018 01:33:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-wm0-x242.google.com with SMTP id t74so11838953wme.3 for ; Wed, 24 Jan 2018 17:33:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=JGuHLx58nfsu+cqGB471zrcSReSDsGJa3bQSNEszkYI=; b=yPYyL46NQbizq36EA9u0Vkiv2I/LjGEngJHbaUItVbCmGROzpuqjNuTqs7nA9GA68W 6OvAtsB/WTUzJRb3k9JVY8gohHaud9AwOLh++aUK+Kftx/3R+zOwGOzF6hKsD2Gq/Umo e+ZcQ+/jwkj0MVefKcfwzab7ERcC8M6PyIQvc0ztpvq48XsT3dcGDf33gCoc10WmeusU CKeuiShTBJS3GOROsXUo6+Eqjv4hClHjHy6EG/zCMSAyOlJKhsSAqvJl98qKfRAQTl7U STUFIAeyrcWhdm+KSnpS9dLX4EpDbF5Go8Gkjoo9wkgsNmbIwE5a0N2jKuK01FVYIKcD qcRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=JGuHLx58nfsu+cqGB471zrcSReSDsGJa3bQSNEszkYI=; b=bI14NfJQwPDdnCfb2YMGs3q5X9ap9TVKYFH2h8ZKQ/69VOKLLWtOhOyAMCjAf7OlFh zYpmmKhl54k8oSHXHEe67LvLSsQIoJ12zMjisR2nwPwHVuWbz991jZSlIY+Ck2AXvfNk 7kbanJgQHtAFdMMmLHoP7Ufrahoe1orEvvKgXKe71rC5kYDqELZ2f2nAXqT6K5zi3bm2 CRVoSN3acMLOpKq4OSXitiZBiH0lTNG6X2SMyaUpDYnCZ1qHPyoRo612j6+5a+hCkvoB 2iCFaWPXfoLxXZoA4pBjA5mcEh6fZugBYrr4f08tG74dLZtAydty70m+aryAjoqfckTM bjKQ== X-Gm-Message-State: AKwxytcJXwh2GQoyEn7PGPTnRGfL5kyNsghWjmvBMTxo2s5rp63Rb6+P DUJ00abuKxJJ+SyrkyV9M6C2SJfu6Wljx93aYCJPZQ== X-Google-Smtp-Source: AH8x227WaouMqF6H+uAUhS8DB0qT/7wgO50zxMZfwk1WEsY21MDgdgj5JavyA6q6RJttDcfh0gUhCAkxrDVZhmO5XL8= X-Received: by 10.80.171.165 with SMTP id u34mr26583363edc.167.1516844015695; Wed, 24 Jan 2018 17:33:35 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.80.133.195 with HTTP; Wed, 24 Jan 2018 17:33:35 -0800 (PST) X-Originating-IP: [2603:300b:6:5100:e53c:ea1d:7f1f:f74b] Received: by 10.80.133.195 with HTTP; Wed, 24 Jan 2018 17:33:35 -0800 (PST) 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> From: Warner Losh Date: Wed, 24 Jan 2018 18:33:35 -0700 X-Google-Sender-Auth: fRdZbTjsTCVrx7oksrdbSxvkcAg Message-ID: Subject: Re: svn commit: r328313 - head/sys/netpfil/pf To: Kristof Provost Cc: Ian Lepore , Gleb Smirnoff , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.25 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 01:33:37 -0000 On Jan 24, 2018 6:08 PM, "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 =3D &V_pf_idhash[PF_IDHASH(s)]; >> K> + int last; >> K> >> K> if ((flags & PF_ENTER_LOCKED) =3D=3D 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 =3D refcount_release(&s->refs); >> K> + KASSERT(last =3D=3D 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=E2=80=99d really like to keep the KASSERT(), because this is the sort o= f thing that could go wrong, and the assertion would be helpful. I suppose I could wrap last in #ifdef INVARIANTS, but that=E2=80=99s rather= ugly too. Asserting that the refcount is at least 1 when entering pf_release_state() would express the same, but that=E2=80=99s 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. It would be good to have a Coverty cheer sheet so we know how to annotate this stuff correctly in the source. I've started fumbling my way through for the boot loader and devmatch changes... Warner