Thanks for review. I'll update and resend these pathces (probably next week).
2017-04-01 17:51 GMT+03:00 Eric Blake <ebl...@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.a...@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.a...@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 > >