diff mbox

[1/2] net/vmxnet3: fix debug macro pattern for vmxnet3

Message ID 1449220774-17968-1-git-send-email-yanmiaobest@gmail.com
State New
Headers show

Commit Message

Miao Yan Dec. 4, 2015, 9:19 a.m. UTC 
Vmxnet3 uses the following debug macro style:

 #ifdef SOME_DEBUG
 #  define debug(...) do{ printf(...); } while (0)
 # else
 #  define debug(...) do{ } while (0)
 #endif

If SOME_DEBUG is undefined, then format string inside the
debug macro will never be checked by compiler. Code will
likely to break in the future when SOME_DEBUG is enabled
 because of lacking of testing. This patch changes this
to the following:

 #define debug(...) \
  do { if (SOME_DEBUG_ENABLED) printf(...); } while (0)

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
 hw/net/vmxnet_debug.h | 139 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 86 insertions(+), 53 deletions(-)

Comments

Eric Blake Dec. 4, 2015, 1:18 p.m. UTC | #1 
On 12/04/2015 02:19 AM, Miao Yan wrote:
> Vmxnet3 uses the following debug macro style:
> 

When sending a patch series, it's best to include a 0/2 cover letter
(some of the maintainer tooling works better if it can reply to the
cover letter).

Also, this patch depends on your earlier patch "net/vmxnet3.c: fix a
build error when enabling debug output" in order to not cause a
temporary compile failure; you should state that dependency so that the
patches get applied in the correct order.  (Or just make the series be 3
patches, with that one as 1/3)

>  #ifdef SOME_DEBUG
>  #  define debug(...) do{ printf(...); } while (0)
>  # else
>  #  define debug(...) do{ } while (0)
>  #endif
> 
> If SOME_DEBUG is undefined, then format string inside the
> debug macro will never be checked by compiler. Code will
> likely to break in the future when SOME_DEBUG is enabled

s/will/is

>  because of lacking of testing. This patch changes this

s/lacking/lack/

> to the following:
> 
>  #define debug(...) \
>   do { if (SOME_DEBUG_ENABLED) printf(...); } while (0)
> 
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
> ---
>  hw/net/vmxnet_debug.h | 139 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 86 insertions(+), 53 deletions(-)
> 

Changes themselves look okay, so with the commit message grammar improved,

Reviewed-by: Eric Blake <eblake@redhat.com>
Miao Yan Dec. 5, 2015, 7:28 a.m. UTC | #2 
2015-12-04 21:18 GMT+08:00 Eric Blake <eblake@redhat.com>:
> On 12/04/2015 02:19 AM, Miao Yan wrote:
>> Vmxnet3 uses the following debug macro style:
>>
>
> When sending a patch series, it's best to include a 0/2 cover letter
> (some of the maintainer tooling works better if it can reply to the
> cover letter).
>
> Also, this patch depends on your earlier patch "net/vmxnet3.c: fix a
> build error when enabling debug output" in order to not cause a
> temporary compile failure; you should state that dependency so that the
> patches get applied in the correct order.  (Or just make the series be 3
> patches, with that one as 1/3)


I thought trivial patches like these don't need a cover letter. I'll
send v2 with your reviewed-by tag. Thanks.


>
>>  #ifdef SOME_DEBUG
>>  #  define debug(...) do{ printf(...); } while (0)
>>  # else
>>  #  define debug(...) do{ } while (0)
>>  #endif
>>
>> If SOME_DEBUG is undefined, then format string inside the
>> debug macro will never be checked by compiler. Code will
>> likely to break in the future when SOME_DEBUG is enabled
>
> s/will/is


Thanks. Will fix.

>
>>  because of lacking of testing. This patch changes this
>
> s/lacking/lack/


Thanks. Will fix.


>
>> to the following:
>>
>>  #define debug(...) \
>>   do { if (SOME_DEBUG_ENABLED) printf(...); } while (0)
>>
>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>> ---
>>  hw/net/vmxnet_debug.h | 139 +++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 86 insertions(+), 53 deletions(-)
>>
>
> Changes themselves look okay, so with the commit message grammar improved,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
diff mbox

Patch

diff --git a/hw/net/vmxnet_debug.h b/hw/net/vmxnet_debug.h
index 96dae0f..96495db 100644
--- a/hw/net/vmxnet_debug.h
+++ b/hw/net/vmxnet_debug.h
@@ -20,94 +20,127 @@ 
 
 #define VMXNET_DEVICE_NAME "vmxnet3"
 
-/* #define VMXNET_DEBUG_CB */
 #define VMXNET_DEBUG_WARNINGS
 #define VMXNET_DEBUG_ERRORS
-/* #define VMXNET_DEBUG_INTERRUPTS */
-/* #define VMXNET_DEBUG_CONFIG */
-/* #define VMXNET_DEBUG_RINGS */
-/* #define VMXNET_DEBUG_PACKETS */
-/* #define VMXNET_DEBUG_SHMEM_ACCESS */
+
+#undef VMXNET_DEBUG_CB
+#undef VMXNET_DEBUG_INTERRUPTS
+#undef VMXNET_DEBUG_CONFIG
+#undef VMXNET_DEBUG_RINGS
+#undef VMXNET_DEBUG_PACKETS
+#undef VMXNET_DEBUG_SHMEM_ACCESS
+
+#ifdef VMXNET_DEBUG_CB
+#  define VMXNET_DEBUG_CB_ENABLED 1
+#else
+#  define VMXNET_DEBUG_CB_ENABLED 0
+#endif
+
+#ifdef VMXNET_DEBUG_WARNINGS
+#  define VMXNET_DEBUG_WARNINGS_ENABLED 1
+#else
+#  define VMXNET_DEBUG_WARNINGS_ENABLED 0
+#endif
+
+#ifdef VMXNET_DEBUG_ERRORS
+#  define VMXNET_DEBUG_ERRORS_ENABLED 1
+#else
+#  define VMXNET_DEBUG_ERRORS_ENABLED 0
+#endif
+
+#ifdef VMXNET_DEBUG_CONFIG
+#  define VMXNET_DEBUG_CONFIG_ENABLED 1
+#else
+#  define VMXNET_DEBUG_CONFIG_ENABLED 0
+#endif
+
+#ifdef VMXNET_DEBUG_RINGS
+#  define VMXNET_DEBUG_RINGS_ENABLED 1
+#else
+#  define VMXNET_DEBUG_RINGS_ENABLED 0
+#endif
+
+#ifdef VMXNET_DEBUG_PACKETS
+#  define VMXNET_DEBUG_PACKETS_ENABLED 1
+#else
+#  define VMXNET_DEBUG_PACKETS_ENABLED 0
+#endif
+
+#ifdef VMXNET_DEBUG_INTERRUPTS
+#  define VMXNET_DEBUG_INTERRUPTS_ENABLED 1
+#else
+#  define VMXNET_DEBUG_INTERRUPTS_ENABLED 0
+#endif
 
 #ifdef VMXNET_DEBUG_SHMEM_ACCESS
+#  define VMXNET_DEBUG_SHMEM_ACCESS_ENABLED 1
+#else
+#  define VMXNET_DEBUG_SHMEM_ACCESS_ENABLED 0
+#endif
+
 #define VMW_SHPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_SHMEM_ACCESS_ENABLED) {                              \
+            printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+       }                                                                      \
     } while (0)
-#else
-#define VMW_SHPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_CB
 #define VMW_CBPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][CB][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_CB_ENABLED) {                                        \
+            printf("[%s][CB][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_CBPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_PACKETS
 #define VMW_PKPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][PK][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_PACKETS_ENABLED) {                                   \
+            printf("[%s][PK][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_PKPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_WARNINGS
 #define VMW_WRPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][WR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_WARNINGS_ENABLED) {                                  \
+            printf("[%s][WR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_WRPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_ERRORS
 #define VMW_ERPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][ER][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_ERRORS_ENABLED) {                                    \
+            printf("[%s][ER][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_ERPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_INTERRUPTS
 #define VMW_IRPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][IR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_INTERRUPTS_ENABLED) {                                \
+            printf("[%s][IR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_IRPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_CONFIG
 #define VMW_CFPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_CONFIG_ENABLED) {                                    \
+            printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_CFPRN(fmt, ...) do {} while (0)
-#endif
 
-#ifdef VMXNET_DEBUG_RINGS
 #define VMW_RIPRN(fmt, ...)                                                   \
     do {                                                                      \
-        printf("[%s][RI][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,       \
-            ## __VA_ARGS__);                                                  \
+        if (VMXNET_DEBUG_RINGS_ENABLED) {                                     \
+            printf("[%s][RI][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,   \
+                ## __VA_ARGS__);                                              \
+        }                                                                     \
     } while (0)
-#else
-#define VMW_RIPRN(fmt, ...) do {} while (0)
-#endif
 
 #define VMXNET_MF       "%02X:%02X:%02X:%02X:%02X:%02X"
 #define VMXNET_MA(a)    (a)[0], (a)[1], (a)[2], (a)[3], (a)[4], (a)[5]