Date: Wed, 24 Jan 2018 18:33:35 -0700 From: Warner Losh <imp@bsdimp.com> To: Kristof Provost <kp@freebsd.org> Cc: Ian Lepore <ian@freebsd.org>, 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: <CANCZdfowMis2qxc0vd713S-GJBFp%2B7JZESg03V9QQpXD2ZDdDw@mail.gmail.com> 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 Jan 24, 2018 6:08 PM, "Kristof Provost" <kp@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfowMis2qxc0vd713S-GJBFp%2B7JZESg03V9QQpXD2ZDdDw>