Re: [dpdk-dev] [PATCH] ethdev: add function name to log message

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Ferruh,

On Tue, Oct 16, 2018 at 04:55:43PM +0100, Ferruh Yigit wrote:
> On 10/12/2018 3:54 PM, Stephen Hemminger wrote:
> > On Fri, 12 Oct 2018 14:43:57 +0200
> > Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> > 
> >> On Fri, Oct 12, 2018 at 11:45:01AM +0100, Ferruh Yigit wrote:
> >>> On 10/12/2018 11:42 AM, Ferruh Yigit wrote:  
> >>>> On 10/11/2018 6:59 PM, Stephen Hemminger wrote:  
> >>>>> @@ -161,8 +161,9 @@ extern "C" {
> >>>>>  
> >>>>>  extern int rte_eth_dev_logtype;
> >>>>>  
> >>>>> -#define RTE_ETHDEV_LOG(level, ...) \
> >>>>> -	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
> >>>>> +#define RTE_ETHDEV_LOG(level, fmt, ...)		\
> >>>>> +	rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, \
> >>>>> +		"%s():" fmt, __func__, ## __VA_ARGS__)  
> >>>>
> >>>> +1 to adding function name, but
> >>>>
> >>>> failsafe is giving build error [1] with clang because of ## usage [2], that is
> >>>> why I add this as ` "" __VA_ARGS__` at first place but you can't do this trick
> >>>> if __VA_ARGS__ used after fmt.
> >>>>
> >>>> I am not aware of a solution for this, __VA_OPT__(,) also didn't worked with clang.  
> >>>
> >>> +cc Adrien & Gaetan,
> >>>
> >>> I saw Adrien put some "workaround" to this for mlx5  
> >>
> >> Yes, through RTE_FMT() (rte_common.h). Something like this:
> >>
> >>  #define RTE_ETHDEV_LOG(level, fmt, ...) \
> >>      rte_log(RTE_LOG_ ## level, \
> >>              rte_eth_dev_logtype, \
> >>              "%s():" fmt, \
> >>              __func__, \
> >>              ## __VA_ARGS__)
> >>
> >> Can be rewritten like that:
> >>
> >>  #define RTE_ETHDEV_LOG(level, ...) \
> >>      rte_log(RTE_LOG_ ## level, \
> >>              rte_eth_dev_logtype, \
> >>              RTE_FMT("%s():" RTE_FMT_HEAD(__VA_ARGS__,), \
> >>                      __func__, \
> >>                      RTE_FMT_TAIL(__VA_ARGS__,)))
> >>
> >> Although not too pretty and convenient, it does the job. In short:
> >>
> >> - Remove "fmt" argument from prototype.
> >> - Enclose format string and its arguments in RTE_FMT().
> >> - Replace "fmt" with RTE_FMT_HEAD(__VA_ARGS__,).
> >> - Replace "## __VA_ARGS__" with RTE_FMT_TAIL(__VA_ARGS__,).
> >> - Yes, trailing commas are mandatory in RTE_FMT_(HEAD|TAIL)().
> >> - Note it quietly appends a dummy "%.0s" argument to the format string.
> >>
> >>>> [1]
> >>>> .../build/include/rte_ethdev.h:166:26: error: token pasting of ',' and
> >>>> __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
> >>>>                 "%s():" fmt, __func__, ## __VA_ARGS__)
> >>>>                                        ^
> >>>>
> >>>> [2]
> >>>> This seems because of "-pedantic" argument driver uses, and other PMDs using
> >>>> "-pedantic", like mlx,  will have same error although they are disable by
> >>>> default and error not observed in default build.
> >>>>   
> >>>   
> >>
> > 
> > Since zero varadic macros is a GNU extension, maybe just adding GNU source?
> 
> I think `-pedantic` is preventing using extension whether we have `_GNU_SOURCE`
> or not.
> 
> Gaetan,
> 
> Why we have `-pedantic` option enabled for failsafe?

Because I wanted to enforce it in failsafe, and core DPDK headers were
compatible so it wasn't too much of a hassle.

If rte_ethdev.h is meant to lose this compatibility, then pedantic will
be disabled upon including it. But I guess if it was kept compatible
until now, then it was a deliberate effort?

-- 
Gaëtan Rivet
6WIND