When DBG() is used in a simple if-else, the resulting code path
currently depends on the definition of DBG(). Inserting the statement in
a "do { ... } while (0)" prevents this possible misuse.
Signed-off-by: Niels de Vos <[email protected]>
---
Note, I have not found any offenders, but a mistake can easily be made.
The following example shows what can go wrong if little intention is
paid to the definition of the DBG() macro.
Example:
if something_went_wrong()
DBG("oh no, something went wrong!\n");
else
printk("all went fine\n");
Old result where the else is placed inside the first if-statment:
if something_went_wrong() {
if (omapfb_debug) {
printk(KERN_DEBUG "oh no, something went wrong!\n");
} else {
printk("all went fine\n");
}
}
New result where the else is an alternative to the first if-statement:
if something_went_wrong() {
do {
if (omapfb_debug)
printk(KERN_DEBUG "oh no, something went wrong!\n");
} while (0);
} else {
printk("all went fine\n");
}
---
drivers/video/omap2/omapfb/omapfb.h | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 1305fc9..a01b0bb 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -34,8 +34,10 @@
#ifdef DEBUG
extern unsigned int omapfb_debug;
#define DBG(format, ...) \
- if (omapfb_debug) \
- printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
+ do { \
+ if (omapfb_debug) \
+ printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
+ while (0)
#else
#define DBG(format, ...)
#endif
--
1.7.4.4
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of Niels de Vos
> Sent: Tuesday, May 10, 2011 2:51 PM
> To: [email protected]
> Cc: [email protected];
> [email protected]; Niels de Vos
> Subject: [PATCH] omap2/omapfb: make DBG() more resistant in
> if-else constructions
>
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the
> statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <[email protected]>
>
> ---
> Note, I have not found any offenders, but a mistake can
> easily be made.
> The following example shows what can go wrong if little intention is
> paid to the definition of the DBG() macro.
>
> Example:
> if something_went_wrong()
> DBG("oh no, something went wrong!\n");
> else
> printk("all went fine\n");
>
> Old result where the else is placed inside the first if-statment:
> if something_went_wrong() {
> if (omapfb_debug) {
> printk(KERN_DEBUG "oh no, something
> went wrong!\n");
> } else {
> printk("all went fine\n");
> }
> }
>
> New result where the else is an alternative to the first if-statement:
> if something_went_wrong() {
> do {
> if (omapfb_debug)
> printk(KERN_DEBUG "oh no,
> something went wrong!\n");
> } while (0);
> } else {
> printk("all went fine\n");
> }
> ---
> drivers/video/omap2/omapfb/omapfb.h | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/omap2/omapfb/omapfb.h
> b/drivers/video/omap2/omapfb/omapfb.h
> index 1305fc9..a01b0bb 100644
> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h
> @@ -34,8 +34,10 @@
> #ifdef DEBUG
> extern unsigned int omapfb_debug;
> #define DBG(format, ...) \
> - if (omapfb_debug) \
> - printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
> + do { \
> + if (omapfb_debug) \
> + printk(KERN_DEBUG "OMAPFB: " format, ##
> __VA_ARGS__); \
> + while (0)
A real good find. Wondering if it really didn't create any problems so far...
~sanjeev
> #else
> #define DBG(format, ...)
> #endif
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> -
On Tue, May 10, 2011 at 11:20, Niels de Vos <[email protected]> wrote:
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <[email protected]>
> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h
> @@ -34,8 +34,10 @@
> #ifdef DEBUG
> extern unsigned int omapfb_debug;
> #define DBG(format, ...) \
> - if (omapfb_debug) \
> - printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
> + do { \
> + if (omapfb_debug) \
> + printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
> + while (0)
Where's the closing '}'?
> #else
> #define DBG(format, ...)
BTW, no printf()-style format checking here.
> #endif
What about using the standard pr_debug()/dev_dbg() instead?
With dynamic debug, it can be enabled at run time.
As a bonus, you get printf()-style format checking if debugging is disabled.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, May 10, 2011 at 10:42 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Tue, May 10, 2011 at 11:20, Niels de Vos <[email protected]> wrote:
>> When DBG() is used in a simple if-else, the resulting code path
>> currently depends on the definition of DBG(). Inserting the statement in
>> a "do { ... } while (0)" prevents this possible misuse.
>>
>> Signed-off-by: Niels de Vos <[email protected]>
>
>> --- a/drivers/video/omap2/omapfb/omapfb.h
>> +++ b/drivers/video/omap2/omapfb/omapfb.h
>> @@ -34,8 +34,10 @@
>> #ifdef DEBUG
>> extern unsigned int omapfb_debug;
>> #define DBG(format, ...) \
>> - if (omapfb_debug) \
>> - printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
>> + do { \
>> + if (omapfb_debug) \
>> + printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
>> + while (0)
>
> Where's the closing '}'?
Good catch! That's in a "fixup!" that I forgot to squash :-/
I'll post an update version in a bit.
>> #else
>> #define DBG(format, ...)
>
> BTW, no printf()-style format checking here.
>
>> #endif
>
> What about using the standard pr_debug()/dev_dbg() instead?
> With dynamic debug, it can be enabled at run time.
> As a bonus, you get printf()-style format checking if debugging is disabled.
I think removing DBG() and the omapfb_debug module-parameter is surely
a good thing. Unfortunately DBG() is used quite a bit in the code and
replacing them 'll take some more time. I don't know yet when I find
some time to do and test that.
Thanks for the pointers,
Niels
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
When DBG() is used in a simple if-else, the resulting code path
currently depends on the definition of DBG(). Inserting the statement in
a "do { ... } while (0)" prevents this possible misuse.
Signed-off-by: Niels de Vos <[email protected]>
---
V2: add the missing closing }
Note, I have not found any offenders, but a mistake can easily be made.
The following example shows what can go wrong if little intention is
paid to the definition of the DBG() macro.
Example:
if something_went_wrong()
DBG("oh no, something went wrong!\n");
else
printk("all went fine\n");
Old result where the else is placed inside the first if-statment:
if something_went_wrong() {
if (omapfb_debug) {
printk(KERN_DEBUG "oh no, something went wrong!\n");
} else {
printk("all went fine\n");
}
}
New result where the else is an alternative to the first if-statement:
if something_went_wrong() {
do {
if (omapfb_debug)
printk(KERN_DEBUG "oh no, something went wrong!\n");
} while (0);
} else {
printk("all went fine\n");
}
---
drivers/video/omap2/omapfb/omapfb.h | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 1305fc9..456c586 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -34,8 +34,10 @@
#ifdef DEBUG
extern unsigned int omapfb_debug;
#define DBG(format, ...) \
- if (omapfb_debug) \
- printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
+ do { \
+ if (omapfb_debug) \
+ printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
+ } while (0)
#else
#define DBG(format, ...)
#endif
--
1.7.4.4
On Tue, 2011-05-10 at 11:42 +0200, Geert Uytterhoeven wrote:
> What about using the standard pr_debug()/dev_dbg() instead?
> With dynamic debug, it can be enabled at run time.
> As a bonus, you get printf()-style format checking if debugging is disabled.
Yes, dev_dbg & co. would be better.
However, one thing I dislike about them is the extra stuff they print.
For example, for omapfb and omapdss dev_dbg will print:
omapfb omapfb: foo
omapdss_dss omapdss_dss: foo
I originally added the debug macros to omapdss to be able to
automatically print the DSS module name, as at that point there was only
one big omapdss device. And I guess I just followed with similar macro
in omapfb also. But I believe both omapdss and omapfb should be changed
to dev_* prints sometime soon.
Tomi
On Tue, May 10, 2011 at 14:08, Tomi Valkeinen <[email protected]> wrote:
> On Tue, 2011-05-10 at 11:42 +0200, Geert Uytterhoeven wrote:
>> What about using the standard pr_debug()/dev_dbg() instead?
>> With dynamic debug, it can be enabled at run time.
>> As a bonus, you get printf()-style format checking if debugging is disabled.
>
> Yes, dev_dbg & co. would be better.
>
> However, one thing I dislike about them is the extra stuff they print.
> For example, for omapfb and omapdss dev_dbg will print:
>
> omapfb omapfb: foo
> omapdss_dss omapdss_dss: foo
>
> I originally added the debug macros to omapdss to be able to
> automatically print the DSS module name, as at that point there was only
> one big omapdss device. And I guess I just followed with similar macro
> in omapfb also. But I believe both omapdss and omapfb should be changed
> to dev_* prints sometime soon.
If you don't want the extra baggage, do
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
and use pr_debug().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, 2011-05-10 at 12:07 +0100, Niels de Vos wrote:
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <[email protected]>
Thanks, I'll add this to the dss2 tree.
Tomi