[PATCH 2/2] staging: brcm80211: replace asserts close to Mac80211 interface
error27 at gmail.com
Fri Mar 25 09:56:39 PDT 2011
On Fri, Mar 25, 2011 at 01:10:59PM +0100, Roland Vossen wrote:
> >This is sort of ugly even after we remove the duplicate likely(). Why
> >don't you just replace the BUG_ON() with a WARN_ON(1);
> Because the WARN_ON will give the user a hint of what condition
> failed (vs logging just the source file and line #). But agreed that
> the condition is more difficult to read, so I will take your
It doesn't though... WARN_ON() just gives you the file and line number.
Here is an example:
That's why it's best to not combine conditions but to test each one on
a separate line.
> Well, that seems to be based on the assumption that reviews will
> catch all coding errors, thus run-time checking is not needed
> anymore. I prefer to build a bit more feedback in the code,
> experience has taught me that some run time checks on critical spots
> can save a lot of debug work.
There is some kind of trade-off, yes. But I already didn't like the
checks and this patch just makes them uglier and more complicated.
The wl_free() function is never called with a NULL parameter. I can't
think of a way to handle a NULL parameter that makes sense. If you
really want to put a check there, then it would be more readable to just
put a "WARN_ON(!wl);" instead of the "if (WARN_ON(!wl) return;".
Adding the return is extra code added to handle conditions that can
never be true. It's not even effective because obviously the code is so
buggy at this point that there is no way to recover.
Just calling WARN_ON() without returning would mean that we'd hit a NULL
dereference and the kernel would oops. An oops doesn't take down the
system. It would kill whichever program triggered the oops and
afterward the driver would be dead. But the driver was going to be dead
anyway because of the bug.
BUG_ON() is the wrong idea as well because that means we can't close our
documents and save everything before doing a reboot. (BUG_ON() should
be used in places where continuing would result in filesystem
There are basically three types conditions where people add WARN_ON()s.
1) Impossible conditions.
2) Things which are possible if people write buggy code.
3) Things which should be impossible but we're not sure.
We shouldn't check for impossible things.
It's good to check for buggy code, but it's better to focus on subtle
things that might be missed by a code review. Btw, the core kernel
won't be buggy, or if it is, we can't fix it so there is no point in
When we're writing new code, then we add a ton of debug code because
we're not sure about how functions are called. This is messy and so
we should eventually learn how the code works and removed the debug
> >>@@ -5173,27 +5185,30 @@ wlc_sendpkt_mac80211(struct wlc_info *wlc, struct sk_buff *sdu,
> >> struct scb *scb =&global_scb;
> >> struct ieee80211_hdr *d11_header = (struct ieee80211_hdr *)(sdu->data);
> >>- ASSERT(sdu);
> >>+ if (unlikely(WARN_ON(sdu == NULL)))
> >>+ goto wlc_sendpkt_fail;
> >Just return directly.
> I thought it would be better coding practice if all functions have
> only one exit point. The CodingStyle doc 'Chapter 7: Centralized
> exiting of functions' seems to agree.
The CodingStyle doc is talking about if you have a kfree() or an unlock
before the return. When I see a goto, I always assume that there is a
unlock down there so I have to scroll down. The goto is misleading.
> >>+ WARN_ON(p->next != NULL || p->prev != NULL);
> >If you break it up, it's more readable and if you ever trigger the
> >warning then you'll know which one is non-NULL.
> > WARN_ON(p->next);
> > WARN_ON(p->prev);
> Agree. An argument could be that combining the statements gives less
> CPU overhead, but wlc_recvctl() is not called frequent enough to
> warrant that.
I worry that you're focused way too much on the wrong things. The main
thing this driver needs is to be more readable so it can get merged out
More information about the devel