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 > suggestion. > It doesn't though... WARN_ON() just gives you the file and line number. Here is an example: http://marc.info/?l=user-mode-linux-devel&m=130036473931064&w=2 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 corruption.) 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 checking. 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 code. > >>@@ -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 from staging. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
