virtio-mmio: Always compile debug prints

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

Posted by Boxuan Li 5 years ago

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.

Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>
---
 hw/virtio/virtio-mmio.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

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
-
-#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
+
+#define DPRINTF(fmt, ...)                                            \
+    do {                                                             \
+        if (DEBUG_VIRTIO_MMIO) {                                     \
+            fprintf(stderr, "virtio_mmio: " fmt , ## __VA_ARGS__);   \
+        }                                                            \
+    } while (0)
 
 /* QOM macros */
 /* virtio-mmio-bus */
-- 
2.13.2

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

Posted by Philippe Mathieu-Daudé 5 years ago

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).

You can read more about tracing in docs/devel/tracing.txt,

and I recomment you to look at the following recent convertions in the
repository history:

commit 8d83cbf1015f547cd9336881e6b62ae2ca293849
Author: Greg Kurz <groug@kaod.org>
Date:   Fri Apr 5 10:05:24 2019 +0200

    target/ppc/kvm: Convert DPRINTF to traces

commit dd849ef2c9d57a329c6001c58dbdf49de712349c
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Thu Feb 21 18:17:46 2019 +0000

    hw/timer/pl031: Convert to using trace events

    Convert the debug printing in the PL031 device to use trace events,

Regards,

Phil.

> Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>
> ---
>  hw/virtio/virtio-mmio.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> 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
> -
> -#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
> +
> +#define DPRINTF(fmt, ...)                                            \
> +    do {                                                             \
> +        if (DEBUG_VIRTIO_MMIO) {                                     \
> +            fprintf(stderr, "virtio_mmio: " fmt , ## __VA_ARGS__);   \
> +        }                                                            \
> +    } while (0)
>  
>  /* QOM macros */
>  /* virtio-mmio-bus */
>

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

Posted by Eric Blake 5 years ago

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

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

Posted by LI, BO XUAN 5 years ago

Hi Eric and Phil,

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

Eric Blake <eblake@redhat.com> 於 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