[Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Miao Yan]

Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
This will cause build error when debug level is raised in
vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).

Use VMXNET_MF and VXMNET_MA instead.

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
 hw/net/vmxnet3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5e3a233..ea3d9b7 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
 
     s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
 
-    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
+    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
 
     s->nic = qemu_new_nic(&net_vmxnet3_info, &s->conf,
                           object_get_typename(OBJECT(s)),
-- 
2.1.4

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Dmitry Fleytman]

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

Acked-by: Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>

> On 3 Dec 2015, at 07:08 AM, Miao Yan <yanmiaobest@gmail.com> wrote:
> 
> Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
> This will cause build error when debug level is raised in
> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
> 
> Use VMXNET_MF and VXMNET_MA instead.
> 
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
> ---
> hw/net/vmxnet3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5e3a233..ea3d9b7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
> 
>     s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
> 
> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
> 
>     s->nic = qemu_new_nic(&net_vmxnet3_info, &s->conf,
>                           object_get_typename(OBJECT(s)),
> -- 
> 2.1.4
> 


[-- Attachment #2: Type: text/html, Size: 2129 bytes --]

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Eric Blake]

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

On 12/02/2015 10:08 PM, Miao Yan wrote:
> Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
> This will cause build error when debug level is raised in
> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
> 
> Use VMXNET_MF and VXMNET_MA instead.
> 
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
> ---
>  hw/net/vmxnet3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5e3a233..ea3d9b7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
>  
>      s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
>  
> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));

This is a classic example of why dead code debug statements are evil.

You should consider also providing a patch to hw/net/vmxnet_debug.h to
fix ALL of the broken debug macros in that file to instead use a sane
pattern, so that the format string is ALWAYS compiled and just optimized
out when debugging is disabled.

Here's a conversion of one of the macros for an example of what to do:

Instead of:

> #ifdef VMXNET_DEBUG_CONFIG
> #define VMW_CFPRN(fmt, ...)                                                 \
>     do {                                                                    \
>         printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,     \
>             ## __VA_ARGS__);                                                \
>     } while (0)
> #else
> #define VMW_CFPRN(fmt, ...) do {} while (0)
> #endif

you should do:

#ifdef VMXNET_DEBUG_CONFIG
# define VMXNET_DEBUG_CONFIG_FLAG 1
#else
# define VMXNET_DEBUG_CONFIG_FLAG 0
#endif
#define VMW_CFPRN(fmt, ...)                                       \
    do {                                                          \
        if (VMXNET_DEBUG_CONFIG_FLAG) {                           \
            printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, \
                   __func__, ## __VA_ARGS__);                     \
        }                                                         \
    } while (0);

With that pattern, VMW_CFPRN() will now always check that its arguments
can compile, even though it has no impact to the code size when
VMXNET_DEBUG_CONFIG is not defined.  Note that once you repair all of
the broken macros (I count 8 in that file), you may have some fallout of
other broken dead code that needs to be fixed.

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

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/02/2015 10:08 PM, Miao Yan wrote:
>> Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
>> This will cause build error when debug level is raised in
>> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
>> 
>> Use VMXNET_MF and VXMNET_MA instead.
>> 
>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>> ---
>>  hw/net/vmxnet3.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 5e3a233..ea3d9b7 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
>>  
>>      s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
>>  
>> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
>> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
>
> This is a classic example of why dead code debug statements are evil.
>
> You should consider also providing a patch to hw/net/vmxnet_debug.h to
> fix ALL of the broken debug macros in that file to instead use a sane
> pattern, so that the format string is ALWAYS compiled and just optimized
> out when debugging is disabled.
>
> Here's a conversion of one of the macros for an example of what to do:
>
> Instead of:
>
>> #ifdef VMXNET_DEBUG_CONFIG
>> #define VMW_CFPRN(fmt, ...)                                                 \
>>     do {                                                                    \
>>         printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,     \
>>             ## __VA_ARGS__);                                                \
>>     } while (0)
>> #else
>> #define VMW_CFPRN(fmt, ...) do {} while (0)
>> #endif
>
> you should do:
>
> #ifdef VMXNET_DEBUG_CONFIG
> # define VMXNET_DEBUG_CONFIG_FLAG 1
> #else
> # define VMXNET_DEBUG_CONFIG_FLAG 0
> #endif
> #define VMW_CFPRN(fmt, ...)                                       \
>     do {                                                          \
>         if (VMXNET_DEBUG_CONFIG_FLAG) {                           \
>             printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, \
>                    __func__, ## __VA_ARGS__);                     \
>         }                                                         \
>     } while (0);
>
> With that pattern, VMW_CFPRN() will now always check that its arguments
> can compile, even though it has no impact to the code size when
> VMXNET_DEBUG_CONFIG is not defined.  Note that once you repair all of
> the broken macros (I count 8 in that file), you may have some fallout of
> other broken dead code that needs to be fixed.

You may want to consider tracepoints instead of hand-rolled debugging
macros: docs/tracing.txt.

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Jason Wang]

On 12/03/2015 03:24 PM, Dmitry Fleytman wrote:
> Acked-by: Dmitry Fleytman <dmitry@daynix.com <mailto:dmitry@daynix.com>>

Applied in my -net for 2.5.

Thanks

>
>> On 3 Dec 2015, at 07:08 AM, Miao Yan <yanmiaobest@gmail.com
>> <mailto:yanmiaobest@gmail.com>> wrote:
>>
>> Macro MAC_FMT and MAC_ARG are not defined, but used in
>> vmxnet3_net_init().
>> This will cause build error when debug level is raised in
>> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
>>
>> Use VMXNET_MF and VXMNET_MA instead.
>>
>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com
>> <mailto:yanmiaobest@gmail.com>>
>> ---
>> hw/net/vmxnet3.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 5e3a233..ea3d9b7 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
>>
>>     s->link_status_and_speed = VMXNET3_LINK_SPEED |
>> VMXNET3_LINK_STATUS_UP;
>>
>> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
>> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
>>
>>     s->nic = qemu_new_nic(&net_vmxnet3_info, &s->conf,
>>                           object_get_typename(OBJECT(s)),
>> -- 
>> 2.1.4
>>
>

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[阎淼]

2015-12-04 14:58 GMT+08:00 阎淼 <yanmiaobest@gmail.com>:
> 2015-12-03 23:59 GMT+08:00 Eric Blake <eblake@redhat.com>:
>> On 12/02/2015 10:08 PM, Miao Yan wrote:
>>> Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
>>> This will cause build error when debug level is raised in
>>> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
>>>
>>> Use VMXNET_MF and VXMNET_MA instead.
>>>
>>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>>> ---
>>>  hw/net/vmxnet3.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index 5e3a233..ea3d9b7 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
>>>
>>>      s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
>>>
>>> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
>>> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
>>
>> This is a classic example of why dead code debug statements are evil.
>>
>> You should consider also providing a patch to hw/net/vmxnet_debug.h to
>> fix ALL of the broken debug macros in that file to instead use a sane
>> pattern, so that the format string is ALWAYS compiled and just optimized
>> out when debugging is disabled.
>>
>> Here's a conversion of one of the macros for an example of what to do:
>
>
> Thanks for the review. Will prepare a patch for it .


Oops, cc list dropped by mistake. Add them back.


>
>
>
>>
>> Instead of:
>>
>>> #ifdef VMXNET_DEBUG_CONFIG
>>> #define VMW_CFPRN(fmt, ...)                                                 \
>>>     do {                                                                    \
>>>         printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,     \
>>>             ## __VA_ARGS__);                                                \
>>>     } while (0)
>>> #else
>>> #define VMW_CFPRN(fmt, ...) do {} while (0)
>>> #endif
>>
>> you should do:
>>
>> #ifdef VMXNET_DEBUG_CONFIG
>> # define VMXNET_DEBUG_CONFIG_FLAG 1
>> #else
>> # define VMXNET_DEBUG_CONFIG_FLAG 0
>> #endif
>> #define VMW_CFPRN(fmt, ...)                                       \
>>     do {                                                          \
>>         if (VMXNET_DEBUG_CONFIG_FLAG) {                           \
>>             printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, \
>>                    __func__, ## __VA_ARGS__);                     \
>>         }                                                         \
>>     } while (0);
>>
>> With that pattern, VMW_CFPRN() will now always check that its arguments
>> can compile, even though it has no impact to the code size when
>> VMXNET_DEBUG_CONFIG is not defined.  Note that once you repair all of
>> the broken macros (I count 8 in that file), you may have some fallout of
>> other broken dead code that needs to be fixed.
>>
>> --
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>>

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[阎淼]

2015-12-04 0:40 GMT+08:00 Markus Armbruster <armbru@redhat.com>:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 12/02/2015 10:08 PM, Miao Yan wrote:
>>> Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
>>> This will cause build error when debug level is raised in
>>> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
>>>
>>> Use VMXNET_MF and VXMNET_MA instead.
>>>
>>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>>> ---
>>>  hw/net/vmxnet3.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index 5e3a233..ea3d9b7 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
>>>
>>>      s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
>>>
>>> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
>>> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
>>
>> This is a classic example of why dead code debug statements are evil.
>>
>> You should consider also providing a patch to hw/net/vmxnet_debug.h to
>> fix ALL of the broken debug macros in that file to instead use a sane
>> pattern, so that the format string is ALWAYS compiled and just optimized
>> out when debugging is disabled.
>>
>> Here's a conversion of one of the macros for an example of what to do:
>>
>> Instead of:
>>
>>> #ifdef VMXNET_DEBUG_CONFIG
>>> #define VMW_CFPRN(fmt, ...)                                                 \
>>>     do {                                                                    \
>>>         printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,     \
>>>             ## __VA_ARGS__);                                                \
>>>     } while (0)
>>> #else
>>> #define VMW_CFPRN(fmt, ...) do {} while (0)
>>> #endif
>>
>> you should do:
>>
>> #ifdef VMXNET_DEBUG_CONFIG
>> # define VMXNET_DEBUG_CONFIG_FLAG 1
>> #else
>> # define VMXNET_DEBUG_CONFIG_FLAG 0
>> #endif
>> #define VMW_CFPRN(fmt, ...)                                       \
>>     do {                                                          \
>>         if (VMXNET_DEBUG_CONFIG_FLAG) {                           \
>>             printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, \
>>                    __func__, ## __VA_ARGS__);                     \
>>         }                                                         \
>>     } while (0);
>>
>> With that pattern, VMW_CFPRN() will now always check that its arguments
>> can compile, even though it has no impact to the code size when
>> VMXNET_DEBUG_CONFIG is not defined.  Note that once you repair all of
>> the broken macros (I count 8 in that file), you may have some fallout of
>> other broken dead code that needs to be fixed.
>
> You may want to consider tracepoints instead of hand-rolled debugging
> macros: docs/tracing.txt.

Hi Markus,

  Thanks for your suggestion. It seems trace functions must have
fixed number of parameters, so, for example,
VMW_PRN("abc") and VMW_PRN("aaa %d", a) would need two trace functions,
right ?

  I looked at the code in vmxnet3.c, IMO not all debug output are
 suitable to be converted to trace points. Maybe I can work out a
 patch to fix the mmio part.

Miao

Re: [Qemu-devel] [PATCH] net/vmxnet3.c: fix a build error when enabling debug output

[Markus Armbruster]

阎淼 <yanmiaobest@gmail.com> writes:

> 2015-12-04 0:40 GMT+08:00 Markus Armbruster <armbru@redhat.com>:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 12/02/2015 10:08 PM, Miao Yan wrote:
>>>> Macro MAC_FMT and MAC_ARG are not defined, but used in vmxnet3_net_init().
>>>> This will cause build error when debug level is raised in
>>>> vmxnet3_debug.h (enable all VMXNET3_DEBUG_xxx).
>>>>
>>>> Use VMXNET_MF and VXMNET_MA instead.
>>>>
>>>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>>>> ---
>>>>  hw/net/vmxnet3.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>>> index 5e3a233..ea3d9b7 100644
>>>> --- a/hw/net/vmxnet3.c
>>>> +++ b/hw/net/vmxnet3.c
>>>> @@ -2044,7 +2044,7 @@ static void vmxnet3_net_init(VMXNET3State *s)
>>>>
>>>>      s->link_status_and_speed = VMXNET3_LINK_SPEED | VMXNET3_LINK_STATUS_UP;
>>>>
>>>> -    VMW_CFPRN("Permanent MAC: " MAC_FMT, MAC_ARG(s->perm_mac.a));
>>>> +    VMW_CFPRN("Permanent MAC: " VMXNET_MF, VMXNET_MA(s->perm_mac.a));
>>>
>>> This is a classic example of why dead code debug statements are evil.
>>>
>>> You should consider also providing a patch to hw/net/vmxnet_debug.h to
>>> fix ALL of the broken debug macros in that file to instead use a sane
>>> pattern, so that the format string is ALWAYS compiled and just optimized
>>> out when debugging is disabled.
>>>
>>> Here's a conversion of one of the macros for an example of what to do:
>>>
>>> Instead of:
>>>
>>>> #ifdef VMXNET_DEBUG_CONFIG
>>>> #define VMW_CFPRN(fmt, ...)                                                 \
>>>>     do {                                                                    \
>>>>         printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,     \
>>>>             ## __VA_ARGS__);                                                \
>>>>     } while (0)
>>>> #else
>>>> #define VMW_CFPRN(fmt, ...) do {} while (0)
>>>> #endif
>>>
>>> you should do:
>>>
>>> #ifdef VMXNET_DEBUG_CONFIG
>>> # define VMXNET_DEBUG_CONFIG_FLAG 1
>>> #else
>>> # define VMXNET_DEBUG_CONFIG_FLAG 0
>>> #endif
>>> #define VMW_CFPRN(fmt, ...)                                       \
>>>     do {                                                          \
>>>         if (VMXNET_DEBUG_CONFIG_FLAG) {                           \
>>>             printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, \
>>>                    __func__, ## __VA_ARGS__);                     \
>>>         }                                                         \
>>>     } while (0);
>>>
>>> With that pattern, VMW_CFPRN() will now always check that its arguments
>>> can compile, even though it has no impact to the code size when
>>> VMXNET_DEBUG_CONFIG is not defined.  Note that once you repair all of
>>> the broken macros (I count 8 in that file), you may have some fallout of
>>> other broken dead code that needs to be fixed.
>>
>> You may want to consider tracepoints instead of hand-rolled debugging
>> macros: docs/tracing.txt.
>
> Hi Markus,
>
>   Thanks for your suggestion. It seems trace functions must have
> fixed number of parameters, so, for example,
> VMW_PRN("abc") and VMW_PRN("aaa %d", a) would need two trace functions,
> right ?

The purpose of debug print helpers is to facilitate enabling and
disabling (sets of related) debug prints at compile time, for debugging
purposes.

Tracepoints are more structured: they are meant for tracing the flow of
control as it passes through points of interest.  In general, you want a
seperate tracepoint for each point of interest, so you can follow the
flow of control in the trace.

Tracepoints can be configured with various backends for various purposes
beyond debugging by developers.  You can ship code with tracepoints
compiled in, but disabled, then enable selected tracepoints in the field
for troubleshooting.

>   I looked at the code in vmxnet3.c, IMO not all debug output are
>  suitable to be converted to trace points. Maybe I can work out a
>  patch to fix the mmio part.

I can't tell you which of your debug prints should be tracepoints.  I
just want to make you aware of tracepoints, so you can decide for
yourself.