Re: [Qemu-devel] [PATCH 02/43] arm: made printf always compile in debug output

[Eric Blake]

On 04/01/2017 08:32 AM, Danil Antonov wrote:
>>From 1671b92e5a4a59d5e38ce754d1d0dde2401c8236 Mon Sep 17 00:00:00 2001
> From: Danil Antonov <address@hidden>
> 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 <address@hidden>
> ---
>  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

signature.asc
Description: OpenPGP digital signature