[PATCH] Staging: crystalhd: crystalhd_misc: improved debug macros

[PATCH] Staging: crystalhd: crystalhd_misc: improved debug macros

[Jorgyano Vieira]

Improvement of debug macros to ensure safe use on if/else statements.

Signed-off-by: Jorgyano Vieira <jorgyano@gmail.com>
---
 drivers/staging/crystalhd/crystalhd_misc.h |   46 +++++++++++++++-------------
 1 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/crystalhd/crystalhd_misc.h b/drivers/staging/crystalhd/crystalhd_misc.h
index 4d61723..e0aa361 100644
--- a/drivers/staging/crystalhd/crystalhd_misc.h
+++ b/drivers/staging/crystalhd/crystalhd_misc.h
@@ -203,26 +203,30 @@ enum _chd_log_levels {
 	BCMLOG_ENTER_LEAVE	= 0x00000008,	/* stack tracking */
 };
 
-#define BCMLOG_ENTER				\
-if (g_linklog_level & BCMLOG_ENTER_LEAVE) {	\
-	printk(KERN_DEBUG "Entered %s\n", __func__);	\
-}
-
-#define BCMLOG_LEAVE				\
-if (g_linklog_level & BCMLOG_ENTER_LEAVE) {	\
-	printk(KERN_DEBUG "Leaving %s\n", __func__);	\
-}
-
-#define BCMLOG(trace, fmt, args...)		\
-if (g_linklog_level & trace) {			\
-	printk(fmt, ##args);			\
-}
-
-#define BCMLOG_ERR(fmt, args...)					\
-do {									\
-	if (g_linklog_level & BCMLOG_ERROR) {				\
-		printk(KERN_ERR "*ERR*:%s:%d: "fmt, __FILE__, __LINE__, ##args);	\
-	}								\
-} while (0);
+#define BCMLOG_ENTER						\
+do {								\
+	if (g_linklog_level & BCMLOG_ENTER_LEAVE)		\
+		printk(KERN_DEBUG "Entered %s\n", __func__);	\
+} while (0)
+
+#define BCMLOG_LEAVE						\
+do {								\
+	if (g_linklog_level & BCMLOG_ENTER_LEAVE)		\
+		printk(KERN_DEBUG "Leaving %s\n", __func__);	\
+} while (0)							\
+
+#define BCMLOG(trace, fmt, args...)	\
+do {					\
+	if (g_linklog_level & trace)	\
+		printk(fmt, ##args);	\
+} while (0)
+
+
+#define BCMLOG_ERR(fmt, args...)				\
+do {								\
+	if (g_linklog_level & BCMLOG_ERROR)			\
+		printk(KERN_ERR "*ERR*:%s:%d: "fmt,		\
+				__FILE__, __LINE__, ##args);	\
+} while (0)
 
 #endif
-- 
1.7.2.5

Re: [PATCH] Staging: crystalhd: crystalhd_misc: improved debug macros

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On Wed, Feb 15, 2012 at 12:20:06AM -0200, Jorgyano Vieira wrote:
> Improvement of debug macros to ensure safe use on if/else statements.
> 

How are the originals unsafe?  The changelog should say how this is
an improvement.  Something like "I put do { ... } while(0) around
the macros so it matches the rest of the kernel."

Really we want to get rid of these.  It would be easy to delete
BCMLOG_ENTER and BCMLOG_LEAVE right now.  They're only used for five
functions.  BCMLOG() is slightly complicated.  BCMLOG_ERR() could
be sed replaced with pr_err().

So could you do that instead?  Send a patch to remove BCMLOG_ENTER
and BCMLOG_LEAVE?  Then send another patch to replace BCMLOG_ERR()
with pr_err().  Take a look at how the pr_fmt macro is used for
this.  Don't actually do it with sed.  Take the time and review each
printk() and notice bugs as you go along.  You could replace them
file by file if you wanted or all at once, which ever is easier for
you.  Then send a third patch to add the do {} while(0) block to
BCMLOG() until someone works up enough motivation to fix it
properly?

regards,
dan carpenter




[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Staging: crystalhd: crystalhd_misc: improved debug macros

[Jorgyano vieira]

On Wed, Feb 15, 2012 at 4:40 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Feb 15, 2012 at 12:20:06AM -0200, Jorgyano Vieira wrote:
>> Improvement of debug macros to ensure safe use on if/else statements.
>>
>
> How are the originals unsafe?
for example If you have something like:
if(foo)
        BCMLOG(...);
else
       do that

> Really we want to get rid of these.  It would be easy to delete
> BCMLOG_ENTER and BCMLOG_LEAVE right now.  They're only used for five
> functions.  BCMLOG() is slightly complicated.  BCMLOG_ERR() could
> be sed replaced with pr_err().
>
> So could you do that instead?  Send a patch to remove BCMLOG_ENTER
> and BCMLOG_LEAVE?  Then send another patch to replace BCMLOG_ERR()
> with pr_err().  Take a look at how the pr_fmt macro is used for
> this.  Don't actually do it with sed.  Take the time and review each
> printk() and notice bugs as you go along.  You could replace them
> file by file if you wanted or all at once, which ever is easier for
> you.  Then send a third patch to add the do {} while(0) block to
> BCMLOG() until someone works up enough motivation to fix it
> properly?
yes, this patch don't make the entire work,
I intend to make the remaining work on the next patches.
thanks for the suggestions.

regards,
Jorgyano Vieira

Re: [PATCH] Staging: crystalhd: crystalhd_misc: improved debug macros

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On Wed, Feb 15, 2012 at 09:26:45AM -0200, Jorgyano vieira wrote:
> yes, this patch don't make the entire work,
> I intend to make the remaining work on the next patches.
> thanks for the suggestions.
> 

Ok.  Fine.  We can remove these in later patches.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]