Thread (6 messages) 6 messages, 3 authors, 2012-11-03

Re: [PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

From: walter harms <hidden>
Date: 2012-11-03 16:26:44
Also in: kernel-janitors, lkml


Am 03.11.2012 15:14, schrieb Julia Lawall:
On Sat, 3 Nov 2012, walter harms wrote:
quoted

Am 03.11.2012 11:58, schrieb Julia Lawall:
quoted
From: Julia Lawall <redacted>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this
transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <redacted>

---
 drivers/net/ethernet/ibm/emac/mal.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/mal.c
b/drivers/net/ethernet/ibm/emac/mal.c
index 479e43e..84c6b6c 100644
--- a/drivers/net/ethernet/ibm/emac/mal.c
+++ b/drivers/net/ethernet/ibm/emac/mal.c
@@ -738,13 +738,11 @@ static int __devexit mal_remove(struct
platform_device *ofdev)
     /* Synchronize with scheduled polling */
     napi_disable(&mal->napi);

-    if (!list_empty(&mal->list)) {
+    if (!list_empty(&mal->list))
         /* This is *very* bad */
-        printk(KERN_EMERG
+        WARN(1, KERN_EMERG
                "mal%d: commac list is not empty on remove!\n",
                mal->index);
-        WARN_ON(1);
-    }

     dev_set_drvdata(&ofdev->dev, NULL);
Hi Julia,
you are removing the {} behin the if. I prefer to be a bit conservative
about {}. There is suggest to keep them because WARN may be expanded in
future (with a second line) and that will cause subtle changes that do
no break the code. (Yes i know it is possible to write macros that
contain savely more than one line.)
WARN is already multi-line, surrounded by ({ }).  It seems to be set up
to be used as an expression.  Is it necessary to assume that it might
someday be changed from safe to unsafe?
my bad,
NTL looks like a candidate for a function.

While looking i have noticed that a lot of drivers define there private "assert" macro.
It is very similar to warn.

(e.g.)
 #define RTL819x_DEBUG
 #ifdef RTL819x_DEBUG
 #define assert(expr) \
        if (!(expr)) {                                  \
                 printk( "Assertion failed! %s,%s,%s,line=%d\n", \
                #expr,__FILE__,__FUNCTION__,__LINE__);          \
        }

re,
 wh
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help