[Qemu-devel] [PATCH] block: always compile-check debug prints

[Qemu-devel] [PATCH] block: always compile-check debug prints

[Zhou Jie]

Files with conditional debug statements should ensure that the printf is
always compiled.
This prevents bitrot of the format string of the debug statement.

Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
---
 block/curl.c     | 13 +++++++------
 block/sheepdog.c | 13 ++++++-------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 5a8f8b6..6655108 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -32,14 +32,15 @@
 #include <curl/curl.h>
 #include "qemu/cutils.h"
 
-// #define DEBUG_CURL
+#define DEBUG_CURL 0
 // #define DEBUG_VERBOSE
 
-#ifdef DEBUG_CURL
-#define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do { } while (0)
-#endif
+#define DPRINTF(fmt, ...)                                            \
+    do {                                                             \
+        if (DEBUG_CURL) {                                            \
+            printf(fmt, ## __VA_ARGS__);                             \
+        }                                                            \
+    } while (0)
 
 #if LIBCURL_VERSION_NUM >= 0x071000
 /* The multi interface timer callback was introduced in 7.16.0 */
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 33e0a33..47cca74 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -293,14 +293,13 @@ static inline size_t count_data_objs(const struct SheepdogInode *inode)
 }
 
 #undef DPRINTF
-#ifdef DEBUG_SDOG
-#define DPRINTF(fmt, args...)                                       \
-    do {                                                            \
-        fprintf(stdout, "%s %d: " fmt, __func__, __LINE__, ##args); \
+#define DEBUG_SDOG 0
+#define DPRINTF(fmt, args...)                                           \
+    do {                                                                \
+        if (DEBUG_SDOG) {                                               \
+            fprintf(stdout, "%s %d: " fmt, __func__, __LINE__, ##args); \
+        }                                                               \
     } while (0)
-#else
-#define DPRINTF(fmt, args...)
-#endif
 
 typedef struct SheepdogAIOCB SheepdogAIOCB;
 
-- 
2.5.5

Re: [Qemu-devel] [PATCH] block: always compile-check debug prints

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

adding qemu-block

On 04/28/2016 02:01 AM, Zhou Jie wrote:
> Files with conditional debug statements should ensure that the printf is
> always compiled.
> This prevents bitrot of the format string of the debug statement.
> 
> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com>
> ---

> +++ b/block/curl.c
> @@ -32,14 +32,15 @@
>  #include <curl/curl.h>
>  #include "qemu/cutils.h"
>  
> -// #define DEBUG_CURL
> +#define DEBUG_CURL 0

Other patches doing similar work keep the user-visible witness as
defined/undefined, so that you can add -DDEBUG_CURL to the CFLAGS
without editing any .c files, and create a secondary witness as the
actual conditional.  See 8c6597123af for an example, where I did:

 #ifdef DEBUG_NBD
-#define TRACE(msg, ...) do { \
-    LOG(msg, ## __VA_ARGS__); \
-} while(0)
+#define DEBUG_NBD_PRINT 1
 #else
-#define TRACE(msg, ...) \
-    do { } while (0)
+#define DEBUG_NBD_PRINT 0
 #endif

+#define TRACE(msg, ...) do { \
+    if (DEBUG_NBD_PRINT) { \

The way you did it, I cannot add -DDEBUG_CURL (because you blindly
redefine it back to 0), but have to edit the .c file.

>  // #define DEBUG_VERBOSE
>  
> -#ifdef DEBUG_CURL
> -#define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)

As long as we're touching this, should we make this print to stderr
instead of stdout?

> +++ b/block/sheepdog.c
> @@ -293,14 +293,13 @@ static inline size_t count_data_objs(const struct SheepdogInode *inode)
>  }
>  
>  #undef DPRINTF
> -#ifdef DEBUG_SDOG
> -#define DPRINTF(fmt, args...)                                       \
> -    do {                                                            \
> -        fprintf(stdout, "%s %d: " fmt, __func__, __LINE__, ##args); \

Again, as long as we're touching this, 'fprintf(stdout,' looks stupid.
Either make it 'printf(' or make the debug go to stderr.

> +#define DEBUG_SDOG 0

Same comment about letting the mere definition of the witness variable
be sufficient

Thanks for doing this; looking forward to v2 (and/or comments from
someone more familiar with whether the block layer should be sending
debug comments to stderr instead of stdout)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]