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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
