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

[Markus Armbruster]

阎淼 <address@hidden> writes:

> 2015-12-04 0:40 GMT+08:00 Markus Armbruster <address@hidden>:
>> Eric Blake <address@hidden> 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 <address@hidden>
>>>> ---
>>>>  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.