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.
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"
