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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]