Message ID | CA+KKJYBN3Gu5jzToi61Qf2Un7jOTzB0HcfREnniUQhTe9B=GGg@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 04/01/2017 08:32 AM, Danil Antonov wrote: >>From 1671b92e5a4a59d5e38ce754d1d0dde2401c8236 Mon Sep 17 00:00:00 2001 > From: Danil Antonov <g.danil.anto@gmail.com> > Date: Wed, 29 Mar 2017 02:09:33 +0300 > Subject: [PATCH 02/43] arm: made printf always compile in debug output > > Wrapped printf calls inside debug macros (DPRINTF) in `if` statement. > This will ensure that printf function will always compile even if debug > output is turned off and, in turn, will prevent bitrot of the format > strings. Again, prefer present tense over past tense in the commit message. > > Signed-off-by: Danil Antonov <g.danil.anto@gmail.com> > --- > hw/arm/strongarm.c | 17 +++++++++++------ > hw/arm/z2.c | 16 ++++++++++------ > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c > index 3311cc3..88368ac 100644 > --- a/hw/arm/strongarm.c > +++ b/hw/arm/strongarm.c > @@ -59,11 +59,16 @@ > - Enhance UART with modem signals > */ > > -#ifdef DEBUG > -# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__) > -#else > -# define DPRINTF(format, ...) do { } while (0) > -#endif > +#ifndef DEBUG > +#define DEBUG 0 > +#endif > + > +#define DPRINTF(fmt, ...) \ > + do { \ > + if (DEBUG) { \ > + fprintf(stderr, fmt, ## __VA_ARGS__); \ Again, changing from stdout to stderr should be mentioned as intentional in the commit message. Note that your change breaks the case of someone that used to do: make CFLAGS=-DDEBUG= because now it results in 'if ()' which is not valid syntax; likewise for someone that used to do CFLAGS=-DDEBUG=yes. (Or put another way, as written, your patch forces someone to use -DDEBUG=0 or -DDEBUG=1). A safer way is to make the 'if' condition a separate variable than the command-line trigger, as in the following, so that regardless of what DEBUG is defined to (including the empty string), you are only using DEBUG in the preprocessor, while the if conditional is under your complete control: #ifdef DEBUG # define DEBUG_PRINT 1 #else # define DEBUG_PRINT 0 #endif #definde DPRINTF()... if (DEBUG_PRINT) > @@ -1022,7 +1027,7 @@ static void > strongarm_uart_update_parameters(StrongARMUARTState > *s) > s->char_transmit_time = (NANOSECONDS_PER_SECOND / speed) * frame_size; > qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp); > > - DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n", > s->chr->label, Oh gross - the old code couldn't even compile correctly when DEBUG was defined (since printf(stderr) tries to treat the stderr pointer as a format string)! This is a definite cleanup, and an argument that your switch from stdout to stderr is correct. > +++ b/hw/arm/z2.c > @@ -27,12 +27,16 @@ > #include "exec/address-spaces.h" > #include "sysemu/qtest.h" > > -#ifdef DEBUG_Z2 > -#define DPRINTF(fmt, ...) \ > - printf(fmt, ## __VA_ARGS__) > -#else > -#define DPRINTF(fmt, ...) > -#endif > +#ifndef DEBUG_Z2 > +#define DEBUG_Z2 0 > +#endif > + > +#define DPRINTF(fmt, ...) \ > + do { \ > + if (DEBUG_Z2) { \ Again, it's best to separate your conditional to be something completely under you control, while still allowing the command line freedom to define DEBUG_Z2 to anything (whether an empty string or a non-numeric value). These types of comments probably apply throughout your entire series, so it may be better if I wait for a v2 before reviewing too many more.
Thanks for review. I'll update and resend these pathces (probably next week). 2017-04-01 17:51 GMT+03:00 Eric Blake <eblake@redhat.com>: > On 04/01/2017 08:32 AM, Danil Antonov wrote: > >>From 1671b92e5a4a59d5e38ce754d1d0dde2401c8236 Mon Sep 17 00:00:00 2001 > > From: Danil Antonov <g.danil.anto@gmail.com> > > Date: Wed, 29 Mar 2017 02:09:33 +0300 > > Subject: [PATCH 02/43] arm: made printf always compile in debug output > > > > Wrapped printf calls inside debug macros (DPRINTF) in `if` statement. > > This will ensure that printf function will always compile even if debug > > output is turned off and, in turn, will prevent bitrot of the format > > strings. > > Again, prefer present tense over past tense in the commit message. > > > > > Signed-off-by: Danil Antonov <g.danil.anto@gmail.com> > > --- > > hw/arm/strongarm.c | 17 +++++++++++------ > > hw/arm/z2.c | 16 ++++++++++------ > > 2 files changed, 21 insertions(+), 12 deletions(-) > > > > diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c > > index 3311cc3..88368ac 100644 > > --- a/hw/arm/strongarm.c > > +++ b/hw/arm/strongarm.c > > @@ -59,11 +59,16 @@ > > - Enhance UART with modem signals > > */ > > > > -#ifdef DEBUG > > -# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__) > > -#else > > -# define DPRINTF(format, ...) do { } while (0) > > -#endif > > +#ifndef DEBUG > > +#define DEBUG 0 > > +#endif > > + > > +#define DPRINTF(fmt, ...) \ > > + do { \ > > + if (DEBUG) { \ > > + fprintf(stderr, fmt, ## __VA_ARGS__); \ > > Again, changing from stdout to stderr should be mentioned as intentional > in the commit message. > > Note that your change breaks the case of someone that used to do: > > make CFLAGS=-DDEBUG= > > because now it results in 'if ()' which is not valid syntax; likewise > for someone that used to do CFLAGS=-DDEBUG=yes. (Or put another way, as > written, your patch forces someone to use -DDEBUG=0 or -DDEBUG=1). A > safer way is to make the 'if' condition a separate variable than the > command-line trigger, as in the following, so that regardless of what > DEBUG is defined to (including the empty string), you are only using > DEBUG in the preprocessor, while the if conditional is under your > complete control: > > #ifdef DEBUG > # define DEBUG_PRINT 1 > #else > # define DEBUG_PRINT 0 > #endif > > #definde DPRINTF()... if (DEBUG_PRINT) > > > @@ -1022,7 +1027,7 @@ static void > > strongarm_uart_update_parameters(StrongARMUARTState > > *s) > > s->char_transmit_time = (NANOSECONDS_PER_SECOND / speed) * > frame_size; > > qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp); > > > > - DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n", > > s->chr->label, > > Oh gross - the old code couldn't even compile correctly when DEBUG was > defined (since printf(stderr) tries to treat the stderr pointer as a > format string)! This is a definite cleanup, and an argument that your > switch from stdout to stderr is correct. > > > > +++ b/hw/arm/z2.c > > @@ -27,12 +27,16 @@ > > #include "exec/address-spaces.h" > > #include "sysemu/qtest.h" > > > > -#ifdef DEBUG_Z2 > > -#define DPRINTF(fmt, ...) \ > > - printf(fmt, ## __VA_ARGS__) > > -#else > > -#define DPRINTF(fmt, ...) > > -#endif > > +#ifndef DEBUG_Z2 > > +#define DEBUG_Z2 0 > > +#endif > > + > > +#define DPRINTF(fmt, ...) \ > > + do { \ > > + if (DEBUG_Z2) { \ > > Again, it's best to separate your conditional to be something completely > under you control, while still allowing the command line freedom to > define DEBUG_Z2 to anything (whether an empty string or a non-numeric > value). > > These types of comments probably apply throughout your entire series, so > it may be better if I wait for a v2 before reviewing too many more. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c index 3311cc3..88368ac 100644 --- a/hw/arm/strongarm.c +++ b/hw/arm/strongarm.c @@ -59,11 +59,16 @@ - Enhance UART with modem signals */ -#ifdef DEBUG -# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__) -#else -# define DPRINTF(format, ...) do { } while (0) -#endif +#ifndef DEBUG +#define DEBUG 0 +#endif + +#define DPRINTF(fmt, ...) \ + do { \ + if (DEBUG) { \ + fprintf(stderr, fmt, ## __VA_ARGS__); \ + } \ + } while (0) static struct { hwaddr io_base; @@ -1022,7 +1027,7 @@ static void strongarm_uart_update_parameters(StrongARMUARTState *s) s->char_transmit_time = (NANOSECONDS_PER_SECOND / speed) * frame_size; qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp); - DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n", s->chr->label, + DPRINTF("%s speed=%d parity=%c data=%d stop=%d\n", s->chr.chr->label, speed, parity, data_bits, stop_bits); } diff --git a/hw/arm/z2.c b/hw/arm/z2.c index 1607cbd..1ee8ed9 100644 --- a/hw/arm/z2.c +++ b/hw/arm/z2.c @@ -27,12 +27,16 @@ #include "exec/address-spaces.h" #include "sysemu/qtest.h" -#ifdef DEBUG_Z2 -#define DPRINTF(fmt, ...) \ - printf(fmt, ## __VA_ARGS__) -#else -#define DPRINTF(fmt, ...) -#endif +#ifndef DEBUG_Z2 +#define DEBUG_Z2 0 +#endif + +#define DPRINTF(fmt, ...) \ + do { \ + if (DEBUG_Z2) { \ + fprintf(stderr, "z2: " fmt, ## __VA_ARGS__); \ + } \ + } while (0) static const struct keymap map[0x100] = { [0 ... 0xff] = { -1, -1 },