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

[LI, BO XUAN]

Hi Eric and Phil,

Thanks for your reviews. I am looking into trace events and I'll send a patch v2 soon.

Eric Blake <address@hidden> 於 2019年4月30日週二 下午11:03寫道：

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.

Got it. Actually, I learned from https://git.qemu.org/?p=qemu.git;a=commitdiff;h=c691320faa6, which is used as an example of Bitrot prevention part in https://wiki.qemu.org/Contribute/BiteSizedTasks. Maybe the wiki page can be updated.

 

>> +            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

Best regards,

Boxuan Li