Re: [Qemu-devel] [PATCH] virtio-mmio: Always compile debug prints

[Eric Blake]

On 4/30/19 5:15 AM, Philippe Mathieu-Daudé wrote:
> Hi Li,
> 
> On 4/28/19 1:02 PM, Boxuan Li wrote:
>> Wrap printf calls inside debug macros (DPRINTF) in `if` statement, and
>> change output to stderr as well. This will ensure that printf function
>> will always compile and prevent bitrot of the format strings.
> 
> There is an effort in QEMU to replace the obsolete DPRINTF() macros by
> trace events (which prevent format strings bitroting).

Trace events are even more powerful than conditional debugs (in that you
can turn them on or off at runtime, instead of compile time). But
incremental improvements are still better than nothing.


>>
>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>> index 5807aa87fe..693b3c9eb4 100644
>> --- a/hw/virtio/virtio-mmio.c
>> +++ b/hw/virtio/virtio-mmio.c
>> @@ -28,15 +28,14 @@
>>  #include "hw/virtio/virtio-bus.h"
>>  #include "qemu/error-report.h"
>>  
>> -/* #define DEBUG_VIRTIO_MMIO */
>> -
>> -#ifdef DEBUG_VIRTIO_MMIO

The old code let a user pass CFLAGS=-DDEBUG_VIRTIO_MMIO to turn things on...

>> -
>> -#define DPRINTF(fmt, ...) \
>> -do { printf("virtio_mmio: " fmt , ## __VA_ARGS__); } while (0)
>> -#else
>> -#define DPRINTF(fmt, ...) do {} while (0)
>> -#endif
>> +#define DEBUG_VIRTIO_MMIO 0

...the new code requires a source code edit. This can be considered a
step backwards in developer friendliness.  Better might be:

#ifdef DEBUG_VIRTIO_MMIO
#define DEBUG_VIRTIO_MMIO_PRINT 1
#else
#define DEBUG_VIRTIO_MMIO_PRINT 0
#endif

>> +
>> +#define DPRINTF(fmt, ...)                                            \
>> +    do {                                                             \
>> +        if (DEBUG_VIRTIO_MMIO) {                                     \

and the corresponding use of DEBUG_VIRTIO_MMIO_PRINT here, so that you
preserve the ability to do a command-line CFLAGS=-D override, rather
than forcing a source code edit.

>> +            fprintf(stderr, "virtio_mmio: " fmt , ## __VA_ARGS__);   \

No space before ,

>> +        }                                                            \
>> +    } while (0)
>>  
>>  /* QOM macros */
>>  /* virtio-mmio-bus */
>>
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature