Re: [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