| Message ID | 1305019249-9898-1-git-send-email-ndevos@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Tomi Valkeinen |
| Headers | show |
> -----Original Message----- > From: linux-omap-owner@vger.kernel.org > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Niels de Vos > Sent: Tuesday, May 10, 2011 2:51 PM > To: linux-omap@vger.kernel.org > Cc: linux-fbdev@vger.kernel.org; > linux-kernel@vger.kernel.org; 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 <ndevos@redhat.com> > > --- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2011 at 11:20, Niels de Vos <ndevos@redhat.com> 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 <ndevos@redhat.com> > --- 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 -- geert@linux-m68k.org 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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2011 at 10:42 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, May 10, 2011 at 11:20, Niels de Vos <ndevos@redhat.com> 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 <ndevos@redhat.com> > >> --- 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 -- geert@linux-m68k.org > > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2011 at 14:08, Tomi Valkeinen <tomi.valkeinen@ti.com> 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 -- geert@linux-m68k.org 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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 <ndevos@redhat.com> --- 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(-)