ethdev: add function name to log message
Checks
Commit Message
While debugging some virtio driver issues, saw:
Invalid port_id=3
but it was not clear which function was logging this because the
same log is being done in multiple places in ethdev.
Trivial fix is to prefix with function name as is done in other
drivers.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
drivers/net/failsafe/failsafe.c | 1 -
lib/librte_ethdev/rte_ethdev.h | 5 +++--
2 files changed, 3 insertions(+), 3 deletions(-)
Comments
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.
[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.
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
>
>
> [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.
>
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.
> >
>
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?
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?
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?
On 10/17/2018 10:26 AM, Gaëtan Rivet wrote:
> 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?
I think it is deliberate effort, I remember Adrien put some patches in the past
to make DPDK headers pedantic compatible by adding __extension__ keywords.
I tried to use __extension__ for ## but not able to make it work, is there a way
to do this?
And I see the reasoning of making DPDK headers pedantic compatible, I think we
should keep them compatible.
@@ -2,7 +2,6 @@
* Copyright 2017 6WIND S.A.
* Copyright 2017 Mellanox Technologies, Ltd
*/
-
#include <rte_alarm.h>
#include <rte_malloc.h>
#include <rte_ethdev_driver.h>
@@ -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__)
struct rte_mbuf;