app/testpmd: send failure logs to stderr
Checks
Commit Message
Running with stdout suppressed or redirected for further processing
is very confusing in the case of errors.
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
app/test-pmd/testpmd.c | 151 ++++++++++++++++++++---------------------
1 file changed, 75 insertions(+), 76 deletions(-)
Comments
Hi
-----Original Message-----
From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Sent: Friday, May 28, 2021 00:25
To: Li, Xiaoyun <xiaoyun.li@intel.com>
Cc: dev@dpdk.org
Subject: [PATCH] app/testpmd: send failure logs to stderr
Running with stdout suppressed or redirected for further processing is very confusing in the case of errors.
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
This patch looks good to me.
But what do you think about make it as a fix and backport to stable branches?
Anyway works for me.
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
<snip>
2.30.2
On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
> Hi
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, May 28, 2021 00:25
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH] app/testpmd: send failure logs to stderr
>
> Running with stdout suppressed or redirected for further processing is very confusing in the case of errors.
>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>
> This patch looks good to me.
> But what do you think about make it as a fix and backport to stable branches?
> Anyway works for me.
I have no strong opinion on the topic.
@Ferruh, what do you think?
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>> Hi
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Friday, May 28, 2021 00:25
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>> Cc: dev@dpdk.org
>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>
>> Running with stdout suppressed or redirected for further processing is very confusing in the case of errors.
>>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>>
>> This patch looks good to me.
>> But what do you think about make it as a fix and backport to stable branches?
>> Anyway works for me.
>
> I have no strong opinion on the topic.
>
> @Ferruh, what do you think?
>
Same here, no strong opinion.
Sending errors to 'stderr' looks correct thing to do, but changing behavior in
the LTS may cause some unexpected side affect, if it is scripted and testpmd
output is parsed etc... For this possibility I would wait for the next LTS.
And because of same reason perhaps a release note can be added.
Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
macro), I don't know if we should switch all logs, including 'printf', to
'TESTPMD_LOG' macro?
Later stdout/sderr can be managed in rte_log level, instead of any specific
logic for the testpmd.
Even there was a defect for this in the rte_log level, that logs should go to
stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>
On Fri, Jun 11, 2021 at 11:35:59AM +0100, Ferruh Yigit wrote:
> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
> > On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
> >> Hi
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Friday, May 28, 2021 00:25
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: [PATCH] app/testpmd: send failure logs to stderr
> >>
> >> Running with stdout suppressed or redirected for further processing is very confusing in the case of errors.
> >>
> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> ---
> >>
> >> This patch looks good to me.
> >> But what do you think about make it as a fix and backport to stable branches?
> >> Anyway works for me.
> >
> > I have no strong opinion on the topic.
> >
> > @Ferruh, what do you think?
> >
>
> Same here, no strong opinion.
> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
> the LTS may cause some unexpected side affect, if it is scripted and testpmd
> output is parsed etc... For this possibility I would wait for the next LTS.
>
There are really 3 options, though:
* apply and backport
* apply now
* apply only to next LTS
I would tend to support the middle option, because sending errors to stderr
is the right thing to do as you say, and I don't think we need to wait for
next LTS to make the change. However, since we don't want to change
behaviour in the older LTS's, I'd suggest not backporting.
/Bruce
On 6/11/21 4:21 PM, Bruce Richardson wrote:
> On Fri, Jun 11, 2021 at 11:35:59AM +0100, Ferruh Yigit wrote:
>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>> Hi
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Friday, May 28, 2021 00:25
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>
>>>> Running with stdout suppressed or redirected for further processing is very confusing in the case of errors.
>>>>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>>>
>>>> This patch looks good to me.
>>>> But what do you think about make it as a fix and backport to stable branches?
>>>> Anyway works for me.
>>>
>>> I have no strong opinion on the topic.
>>>
>>> @Ferruh, what do you think?
>>>
>>
>> Same here, no strong opinion.
>> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
>> the LTS may cause some unexpected side affect, if it is scripted and testpmd
>> output is parsed etc... For this possibility I would wait for the next LTS.
>>
> There are really 3 options, though:
> * apply and backport
> * apply now
> * apply only to next LTS
>
> I would tend to support the middle option, because sending errors to stderr
> is the right thing to do as you say, and I don't think we need to wait for
> next LTS to make the change. However, since we don't want to change
> behaviour in the older LTS's, I'd suggest not backporting.
Many thanks for motivated point of view. I fully agree.
On 6/11/21 1:35 PM, Ferruh Yigit wrote:
> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>> Hi
>>> -----Original Message-----
>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Sent: Friday, May 28, 2021 00:25
>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>> Cc: dev@dpdk.org
>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>
>>> Running with stdout suppressed or redirected for further processing is very confusing in the case of errors.
>>>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> ---
>>>
>>> This patch looks good to me.
>>> But what do you think about make it as a fix and backport to stable branches?
>>> Anyway works for me.
>>
>> I have no strong opinion on the topic.
>>
>> @Ferruh, what do you think?
>>
>
> Same here, no strong opinion.
> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
> the LTS may cause some unexpected side affect, if it is scripted and testpmd
> output is parsed etc... For this possibility I would wait for the next LTS.
So, I guess all agree that backporting to LTS is a bad idea because of
behaviour change.
As I said in a sub-thread I tend to apply in v21.08 since it is a right
thing to do like a fix, but the fix is not that required to be
backported to change behaviour of LTS releases.
> And because of same reason perhaps a release note can be added.
I'll make v2 with release notes added. Good point.
> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
> macro), I don't know if we should switch all logs, including 'printf', to
> 'TESTPMD_LOG' macro?
> Later stdout/sderr can be managed in rte_log level, instead of any specific
> logic for the testpmd.
I think fprintf() is a better option for debug tool, since its
messages should not go to syslog etc. It should go to stdout/stderr
regardless of logging configuration and log level settings.
> Even there was a defect for this in the rte_log level, that logs should go to
> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>
>
>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>
On 6/14/2021 10:26 PM, Andrew Rybchenko wrote:
> On 6/11/21 1:35 PM, Ferruh Yigit wrote:
>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>> Hi
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Friday, May 28, 2021 00:25
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>
>>>> Running with stdout suppressed or redirected for further processing
>>>> is very confusing in the case of errors.
>>>>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>>>
>>>> This patch looks good to me.
>>>> But what do you think about make it as a fix and backport to stable
>>>> branches?
>>>> Anyway works for me.
>>>
>>> I have no strong opinion on the topic.
>>>
>>> @Ferruh, what do you think?
>>>
>>
>> Same here, no strong opinion.
>> Sending errors to 'stderr' looks correct thing to do, but changing
>> behavior in
>> the LTS may cause some unexpected side affect, if it is scripted and
>> testpmd
>> output is parsed etc... For this possibility I would wait for the
>> next LTS.
>
> So, I guess all agree that backporting to LTS is a bad idea because of
> behaviour change.
>
> As I said in a sub-thread I tend to apply in v21.08 since it is a right
> thing to do like a fix, but the fix is not that required to be
> backported to change behaviour of LTS releases.
>
>> And because of same reason perhaps a release note can be added.
>
> I'll make v2 with release notes added. Good point.
>
>> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as
>> 'RTE_LOG'
>> macro), I don't know if we should switch all logs, including
>> 'printf', to
>> 'TESTPMD_LOG' macro?
>> Later stdout/sderr can be managed in rte_log level, instead of any
>> specific
>> logic for the testpmd.
>
> I think fprintf() is a better option for debug tool, since its
> messages should not go to syslog etc. It should go to stdout/stderr
> regardless of logging configuration and log level settings.
For loging, currently we are using multiple API's in testpmd
(TESTPMD_LOG, RTE_LOG, snprintf, printf). I agree with Ferruh, in long
run we should use single loging mechanism where log_level should decide
the output stream.
For now fprintf() seems OK, as we can pass the output stream compared to
printf().
>
>> Even there was a defect for this in the rte_log level, that logs
>> should go to
>> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>>
>>
>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>>
>
Acked-by: Aman Deep Singh <aman.deep.singh@intel.com
<mailto:aman.deep.singh@intel.com>
On 6/14/2021 5:56 PM, Andrew Rybchenko wrote:
> On 6/11/21 1:35 PM, Ferruh Yigit wrote:
>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>> Hi
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Friday, May 28, 2021 00:25
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>
>>>> Running with stdout suppressed or redirected for further processing is very
>>>> confusing in the case of errors.
>>>>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>>>
>>>> This patch looks good to me.
>>>> But what do you think about make it as a fix and backport to stable branches?
>>>> Anyway works for me.
>>>
>>> I have no strong opinion on the topic.
>>>
>>> @Ferruh, what do you think?
>>>
>>
>> Same here, no strong opinion.
>> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
>> the LTS may cause some unexpected side affect, if it is scripted and testpmd
>> output is parsed etc... For this possibility I would wait for the next LTS.
>
> So, I guess all agree that backporting to LTS is a bad idea because of
> behaviour change.
>
> As I said in a sub-thread I tend to apply in v21.08 since it is a right
> thing to do like a fix, but the fix is not that required to be
> backported to change behaviour of LTS releases.
>
>> And because of same reason perhaps a release note can be added.
>
> I'll make v2 with release notes added. Good point.
>
>> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
>> macro), I don't know if we should switch all logs, including 'printf', to
>> 'TESTPMD_LOG' macro?
>> Later stdout/sderr can be managed in rte_log level, instead of any specific
>> logic for the testpmd.
>
> I think fprintf() is a better option for debug tool, since its
> messages should not go to syslog etc. It should go to stdout/stderr
> regardless of logging configuration and log level settings.
>
Why application should not take log configuration and log level settings into
account? I think this is a feature we can benefit.
And for not logging to syslog, I think it is DPDK wide concern, not specific to
testpmd, we should have way to say don't log to syslog or only log error to
syslog etc.. When it is done, using 'TESTPMD_LOG' enables benefiting from that.
>> Even there was a defect for this in the rte_log level, that logs should go to
>> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>>
>>
>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>>
>
On 6/15/21 10:59 AM, Ferruh Yigit wrote:
> On 6/14/2021 5:56 PM, Andrew Rybchenko wrote:
>> On 6/11/21 1:35 PM, Ferruh Yigit wrote:
>>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>>> Hi
>>>>> -----Original Message-----
>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> Sent: Friday, May 28, 2021 00:25
>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>>> Cc: dev@dpdk.org
>>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>>
>>>>> Running with stdout suppressed or redirected for further processing is very
>>>>> confusing in the case of errors.
>>>>>
>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> ---
>>>>>
>>>>> This patch looks good to me.
>>>>> But what do you think about make it as a fix and backport to stable branches?
>>>>> Anyway works for me.
>>>>
>>>> I have no strong opinion on the topic.
>>>>
>>>> @Ferruh, what do you think?
>>>>
>>>
>>> Same here, no strong opinion.
>>> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
>>> the LTS may cause some unexpected side affect, if it is scripted and testpmd
>>> output is parsed etc... For this possibility I would wait for the next LTS.
>>
>> So, I guess all agree that backporting to LTS is a bad idea because of
>> behaviour change.
>>
>> As I said in a sub-thread I tend to apply in v21.08 since it is a right
>> thing to do like a fix, but the fix is not that required to be
>> backported to change behaviour of LTS releases.
>>
>>> And because of same reason perhaps a release note can be added.
>>
>> I'll make v2 with release notes added. Good point.
>>
>>> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
>>> macro), I don't know if we should switch all logs, including 'printf', to
>>> 'TESTPMD_LOG' macro?
>>> Later stdout/sderr can be managed in rte_log level, instead of any specific
>>> logic for the testpmd.
>>
>> I think fprintf() is a better option for debug tool, since its
>> messages should not go to syslog etc. It should go to stdout/stderr
>> regardless of logging configuration and log level settings.
>>
>
> Why application should not take log configuration and log level settings into
> account? I think this is a feature we can benefit.
For me it sounds like an extra way to shoot its own leg.
> And for not logging to syslog, I think it is DPDK wide concern, not specific to
> testpmd, we should have way to say don't log to syslog or only log error to
> syslog etc.. When it is done, using 'TESTPMD_LOG' enables benefiting from that.
Logging configuration should be flexible to support various
logging backends. IMHO, we don't need the flexibility here.
testpmd is a command-line test application and errors should
simply go to stderr. That's it. Since the result is the
same in both ways, my opinion is not strong, I'm just trying
to explain why I slightly prefer suggested way.
I can switch to TESTPMD_LOG() (or define TESTPMD_ERR() and use
it) easily. I just need maintainers decision on it.
>>> Even there was a defect for this in the rte_log level, that logs should go to
>>> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>>>
>>>
>>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>>>
>>
On 6/15/2021 9:14 AM, Andrew Rybchenko wrote:
> On 6/15/21 10:59 AM, Ferruh Yigit wrote:
>> On 6/14/2021 5:56 PM, Andrew Rybchenko wrote:
>>> On 6/11/21 1:35 PM, Ferruh Yigit wrote:
>>>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>>>> Hi
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Friday, May 28, 2021 00:25
>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>>>
>>>>>> Running with stdout suppressed or redirected for further processing is very
>>>>>> confusing in the case of errors.
>>>>>>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> ---
>>>>>>
>>>>>> This patch looks good to me.
>>>>>> But what do you think about make it as a fix and backport to stable branches?
>>>>>> Anyway works for me.
>>>>>
>>>>> I have no strong opinion on the topic.
>>>>>
>>>>> @Ferruh, what do you think?
>>>>>
>>>>
>>>> Same here, no strong opinion.
>>>> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
>>>> the LTS may cause some unexpected side affect, if it is scripted and testpmd
>>>> output is parsed etc... For this possibility I would wait for the next LTS.
>>>
>>> So, I guess all agree that backporting to LTS is a bad idea because of
>>> behaviour change.
>>>
>>> As I said in a sub-thread I tend to apply in v21.08 since it is a right
>>> thing to do like a fix, but the fix is not that required to be
>>> backported to change behaviour of LTS releases.
>>>
>>>> And because of same reason perhaps a release note can be added.
>>>
>>> I'll make v2 with release notes added. Good point.
>>>
>>>> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
>>>> macro), I don't know if we should switch all logs, including 'printf', to
>>>> 'TESTPMD_LOG' macro?
>>>> Later stdout/sderr can be managed in rte_log level, instead of any specific
>>>> logic for the testpmd.
>>>
>>> I think fprintf() is a better option for debug tool, since its
>>> messages should not go to syslog etc. It should go to stdout/stderr
>>> regardless of logging configuration and log level settings.
>>>
>>
>> Why application should not take log configuration and log level settings into
>> account? I think this is a feature we can benefit.
>
> For me it sounds like an extra way to shoot its own leg.
>
Please explain what is the cons of it?
>> And for not logging to syslog, I think it is DPDK wide concern, not specific to
>> testpmd, we should have way to say don't log to syslog or only log error to
>> syslog etc.. When it is done, using 'TESTPMD_LOG' enables benefiting from that.
>
> Logging configuration should be flexible to support various
> logging backends. IMHO, we don't need the flexibility here.
> testpmd is a command-line test application and errors should
> simply go to stderr. That's it. Since the result is the
> same in both ways, my opinion is not strong, I'm just trying
> to explain why I slightly prefer suggested way.
>
Ability to make it less or more verbose seems good opinion to me, just
printf/fprintf doesn't enable it.
And testpmd sometimes used in non-interactive mode for functional testing,
flexible logging can help there too, I think at least.
> I can switch to TESTPMD_LOG() (or define TESTPMD_ERR() and use
> it) easily. I just need maintainers decision on it.
>
>>>> Even there was a defect for this in the rte_log level, that logs should go to
>>>> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>>>>
>>>>
>>>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>>>>
>>>
>
On 6/15/21 11:52 AM, Ferruh Yigit wrote:
> On 6/15/2021 9:14 AM, Andrew Rybchenko wrote:
>> On 6/15/21 10:59 AM, Ferruh Yigit wrote:
>>> On 6/14/2021 5:56 PM, Andrew Rybchenko wrote:
>>>> On 6/11/21 1:35 PM, Ferruh Yigit wrote:
>>>>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>>>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>>>>> Hi
>>>>>>> -----Original Message-----
>>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>> Sent: Friday, May 28, 2021 00:25
>>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>>>>> Cc: dev@dpdk.org
>>>>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>>>>
>>>>>>> Running with stdout suppressed or redirected for further processing is very
>>>>>>> confusing in the case of errors.
>>>>>>>
>>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>> ---
>>>>>>>
>>>>>>> This patch looks good to me.
>>>>>>> But what do you think about make it as a fix and backport to stable branches?
>>>>>>> Anyway works for me.
>>>>>> I have no strong opinion on the topic.
>>>>>>
>>>>>> @Ferruh, what do you think?
>>>>>>
>>>>> Same here, no strong opinion.
>>>>> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
>>>>> the LTS may cause some unexpected side affect, if it is scripted and testpmd
>>>>> output is parsed etc... For this possibility I would wait for the next LTS.
>>>> So, I guess all agree that backporting to LTS is a bad idea because of
>>>> behaviour change.
>>>>
>>>> As I said in a sub-thread I tend to apply in v21.08 since it is a right
>>>> thing to do like a fix, but the fix is not that required to be
>>>> backported to change behaviour of LTS releases.
>>>>
>>>>> And because of same reason perhaps a release note can be added.
>>>> I'll make v2 with release notes added. Good point.
>>>>
>>>>> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
>>>>> macro), I don't know if we should switch all logs, including 'printf', to
>>>>> 'TESTPMD_LOG' macro?
>>>>> Later stdout/sderr can be managed in rte_log level, instead of any specific
>>>>> logic for the testpmd.
>>>> I think fprintf() is a better option for debug tool, since its
>>>> messages should not go to syslog etc. It should go to stdout/stderr
>>>> regardless of logging configuration and log level settings.
>>>>
>>> Why application should not take log configuration and log level settings into
>>> account? I think this is a feature we can benefit.
>> For me it sounds like an extra way to shoot its own leg.
>>
> Please explain what is the cons of it?
Possibility to silent error logs for test tools.
May be it is just mine paranoia.
>>> And for not logging to syslog, I think it is DPDK wide concern, not specific to
>>> testpmd, we should have way to say don't log to syslog or only log error to
>>> syslog etc.. When it is done, using 'TESTPMD_LOG' enables benefiting from that.
>> Logging configuration should be flexible to support various
>> logging backends. IMHO, we don't need the flexibility here.
>> testpmd is a command-line test application and errors should
>> simply go to stderr. That's it. Since the result is the
>> same in both ways, my opinion is not strong, I'm just trying
>> to explain why I slightly prefer suggested way.
>>
> Ability to make it less or more verbose seems good opinion to me, just
> printf/fprintf doesn't enable it.
Yes, for helper tracing and extra information logging. IMHO, no for
error logs.
So, the strong argument here is to use uniform logging in the
code everywhere and hope that if somebody disables/redirects
errors it is really intended.
But in this case all printf's should be converted as well.
> And testpmd sometimes used in non-interactive mode for functional testing,
> flexible logging can help there too, I think at least.
in this case stdout/stderr are simply intercepted and processed.
Yes, it could be a bit easier if we redirect it to an interface which
natively provides messages boundaries - a bit easier to parse/match.
>> I can switch to TESTPMD_LOG() (or define TESTPMD_ERR() and use
>> it) easily. I just need maintainers decision on it.
>>
>>>>> Even there was a defect for this in the rte_log level, that logs should go to
>>>>> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>>>>>
>>>>>
>>>>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
On 6/15/2021 10:00 AM, Andrew Rybchenko wrote:
> On 6/15/21 11:52 AM, Ferruh Yigit wrote:
>> On 6/15/2021 9:14 AM, Andrew Rybchenko wrote:
>>> On 6/15/21 10:59 AM, Ferruh Yigit wrote:
>>>> On 6/14/2021 5:56 PM, Andrew Rybchenko wrote:
>>>>> On 6/11/21 1:35 PM, Ferruh Yigit wrote:
>>>>>> On 6/11/2021 10:19 AM, Andrew Rybchenko wrote:
>>>>>>> On 6/11/21 5:06 AM, Li, Xiaoyun wrote:
>>>>>>>> Hi
>>>>>>>> -----Original Message-----
>>>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>> Sent: Friday, May 28, 2021 00:25
>>>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>>>>>> Cc: dev@dpdk.org
>>>>>>>> Subject: [PATCH] app/testpmd: send failure logs to stderr
>>>>>>>>
>>>>>>>> Running with stdout suppressed or redirected for further processing is very
>>>>>>>> confusing in the case of errors.
>>>>>>>>
>>>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> This patch looks good to me.
>>>>>>>> But what do you think about make it as a fix and backport to stable branches?
>>>>>>>> Anyway works for me.
>>>>>>> I have no strong opinion on the topic.
>>>>>>>
>>>>>>> @Ferruh, what do you think?
>>>>>>>
>>>>>> Same here, no strong opinion.
>>>>>> Sending errors to 'stderr' looks correct thing to do, but changing behavior in
>>>>>> the LTS may cause some unexpected side affect, if it is scripted and testpmd
>>>>>> output is parsed etc... For this possibility I would wait for the next LTS.
>>>>> So, I guess all agree that backporting to LTS is a bad idea because of
>>>>> behaviour change.
>>>>>
>>>>> As I said in a sub-thread I tend to apply in v21.08 since it is a right
>>>>> thing to do like a fix, but the fix is not that required to be
>>>>> backported to change behaviour of LTS releases.
>>>>>
>>>>>> And because of same reason perhaps a release note can be added.
>>>>> I'll make v2 with release notes added. Good point.
>>>>>
>>>>>> Also there is 'TESTPMD_LOG' macro for logs in testpmd, (as well as 'RTE_LOG'
>>>>>> macro), I don't know if we should switch all logs, including 'printf', to
>>>>>> 'TESTPMD_LOG' macro?
>>>>>> Later stdout/sderr can be managed in rte_log level, instead of any specific
>>>>>> logic for the testpmd.
>>>>> I think fprintf() is a better option for debug tool, since its
>>>>> messages should not go to syslog etc. It should go to stdout/stderr
>>>>> regardless of logging configuration and log level settings.
>>>>>
>>>> Why application should not take log configuration and log level settings into
>>>> account? I think this is a feature we can benefit.
>>> For me it sounds like an extra way to shoot its own leg.
>>>
>> Please explain what is the cons of it?
>
> Possibility to silent error logs for test tools.
>
Got your concern, those can do '2> /dev/null' too :)
I was thinking flexibility to increase verbosity and enable debug options, not
for disabling error/warnings.
> May be it is just mine paranoia.
>
>>>> And for not logging to syslog, I think it is DPDK wide concern, not specific to
>>>> testpmd, we should have way to say don't log to syslog or only log error to
>>>> syslog etc.. When it is done, using 'TESTPMD_LOG' enables benefiting from that.
>>> Logging configuration should be flexible to support various
>>> logging backends. IMHO, we don't need the flexibility here.
>>> testpmd is a command-line test application and errors should
>>> simply go to stderr. That's it. Since the result is the
>>> same in both ways, my opinion is not strong, I'm just trying
>>> to explain why I slightly prefer suggested way.
>>>
>> Ability to make it less or more verbose seems good opinion to me, just
>> printf/fprintf doesn't enable it.
>
> Yes, for helper tracing and extra information logging. IMHO, no for
> error logs.
> > So, the strong argument here is to use uniform logging in the
> code everywhere and hope that if somebody disables/redirects
> errors it is really intended.
>
> But in this case all printf's should be converted as well.
>
Yes, I was asking if we should do this. Doesn't need to be in single big patch
but it can be done gradually and by adding new logs as TESTPMD_LOG.
>> And testpmd sometimes used in non-interactive mode for functional testing,
>> flexible logging can help there too, I think at least.
>
> in this case stdout/stderr are simply intercepted and processed.
> Yes, it could be a bit easier if we redirect it to an interface which
> natively provides messages boundaries - a bit easier to parse/match.
>
>>> I can switch to TESTPMD_LOG() (or define TESTPMD_ERR() and use
>>> it) easily. I just need maintainers decision on it.
>>>
>>>>>> Even there was a defect for this in the rte_log level, that logs should go to
>>>>>> stderr: https://bugs.dpdk.org/show_bug.cgi?id=8
>>>>>>
>>>>>>
>>>>>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>
@@ -1126,11 +1126,9 @@ check_nb_rxq(queueid_t rxq)
allowed_max_rxq = get_allowed_max_nb_rxq(&pid);
if (rxq > allowed_max_rxq) {
- printf("Fail: input rxq (%u) can't be greater "
- "than max_rx_queues (%u) of port %u\n",
- rxq,
- allowed_max_rxq,
- pid);
+ fprintf(stderr,
+ "Fail: input rxq (%u) can't be greater than max_rx_queues (%u) of port %u\n",
+ rxq, allowed_max_rxq, pid);
return -1;
}
return 0;
@@ -1176,11 +1174,9 @@ check_nb_txq(queueid_t txq)
allowed_max_txq = get_allowed_max_nb_txq(&pid);
if (txq > allowed_max_txq) {
- printf("Fail: input txq (%u) can't be greater "
- "than max_tx_queues (%u) of port %u\n",
- txq,
- allowed_max_txq,
- pid);
+ fprintf(stderr,
+ "Fail: input txq (%u) can't be greater than max_tx_queues (%u) of port %u\n",
+ txq, allowed_max_txq, pid);
return -1;
}
return 0;
@@ -1251,21 +1247,17 @@ check_nb_rxd(queueid_t rxd)
allowed_max_rxd = get_allowed_max_nb_rxd(&pid);
if (rxd > allowed_max_rxd) {
- printf("Fail: input rxd (%u) can't be greater "
- "than max_rxds (%u) of port %u\n",
- rxd,
- allowed_max_rxd,
- pid);
+ fprintf(stderr,
+ "Fail: input rxd (%u) can't be greater than max_rxds (%u) of port %u\n",
+ rxd, allowed_max_rxd, pid);
return -1;
}
allowed_min_rxd = get_allowed_min_nb_rxd(&pid);
if (rxd < allowed_min_rxd) {
- printf("Fail: input rxd (%u) can't be less "
- "than min_rxds (%u) of port %u\n",
- rxd,
- allowed_min_rxd,
- pid);
+ fprintf(stderr,
+ "Fail: input rxd (%u) can't be less than min_rxds (%u) of port %u\n",
+ rxd, allowed_min_rxd, pid);
return -1;
}
@@ -1336,21 +1328,17 @@ check_nb_txd(queueid_t txd)
allowed_max_txd = get_allowed_max_nb_txd(&pid);
if (txd > allowed_max_txd) {
- printf("Fail: input txd (%u) can't be greater "
- "than max_txds (%u) of port %u\n",
- txd,
- allowed_max_txd,
- pid);
+ fprintf(stderr,
+ "Fail: input txd (%u) can't be greater than max_txds (%u) of port %u\n",
+ txd, allowed_max_txd, pid);
return -1;
}
allowed_min_txd = get_allowed_min_nb_txd(&pid);
if (txd < allowed_min_txd) {
- printf("Fail: input txd (%u) can't be less "
- "than min_txds (%u) of port %u\n",
- txd,
- allowed_min_txd,
- pid);
+ fprintf(stderr,
+ "Fail: input txd (%u) can't be less than min_txds (%u) of port %u\n",
+ txd, allowed_min_txd, pid);
return -1;
}
return 0;
@@ -1396,9 +1384,9 @@ check_nb_hairpinq(queueid_t hairpinq)
allowed_max_hairpinq = get_allowed_max_nb_hairpinq(&pid);
if (hairpinq > allowed_max_hairpinq) {
- printf("Fail: input hairpin (%u) can't be greater "
- "than max_hairpin_queues (%u) of port %u\n",
- hairpinq, allowed_max_hairpinq, pid);
+ fprintf(stderr,
+ "Fail: input hairpin (%u) can't be greater than max_hairpin_queues (%u) of port %u\n",
+ hairpinq, allowed_max_hairpinq, pid);
return -1;
}
return 0;
@@ -1454,7 +1442,8 @@ init_config(void)
ret = update_jumbo_frame_offload(pid);
if (ret != 0)
- printf("Updating jumbo frame offload failed for port %u\n",
+ fprintf(stderr,
+ "Updating jumbo frame offload failed for port %u\n",
pid);
if (!(port->dev_info.tx_offload_capa &
@@ -1628,15 +1617,15 @@ init_fwd_streams(void)
RTE_ETH_FOREACH_DEV(pid) {
port = &ports[pid];
if (nb_rxq > port->dev_info.max_rx_queues) {
- printf("Fail: nb_rxq(%d) is greater than "
- "max_rx_queues(%d)\n", nb_rxq,
- port->dev_info.max_rx_queues);
+ fprintf(stderr,
+ "Fail: nb_rxq(%d) is greater than max_rx_queues(%d)\n",
+ nb_rxq, port->dev_info.max_rx_queues);
return -1;
}
if (nb_txq > port->dev_info.max_tx_queues) {
- printf("Fail: nb_txq(%d) is greater than "
- "max_tx_queues(%d)\n", nb_txq,
- port->dev_info.max_tx_queues);
+ fprintf(stderr,
+ "Fail: nb_txq(%d) is greater than max_tx_queues(%d)\n",
+ nb_txq, port->dev_info.max_tx_queues);
return -1;
}
if (numa_support) {
@@ -1663,7 +1652,8 @@ init_fwd_streams(void)
q = RTE_MAX(nb_rxq, nb_txq);
if (q == 0) {
- printf("Fail: Cannot allocate fwd streams as number of queues is 0\n");
+ fprintf(stderr,
+ "Fail: Cannot allocate fwd streams as number of queues is 0\n");
return -1;
}
nb_fwd_streams_new = (streamid_t)(nb_ports * q);
@@ -2120,8 +2110,8 @@ launch_packet_forwarding(lcore_function_t *pkt_fwd_on_lcore)
diag = rte_eal_remote_launch(pkt_fwd_on_lcore,
fwd_lcores[i], lc_id);
if (diag != 0)
- printf("launch lcore %u failed - diag=%d\n",
- lc_id, diag);
+ fprintf(stderr, "launch lcore %u failed - diag=%d\n",
+ lc_id, diag);
}
}
}
@@ -2223,14 +2213,14 @@ void
dev_set_link_up(portid_t pid)
{
if (rte_eth_dev_set_link_up(pid) < 0)
- printf("\nSet link up fail.\n");
+ fprintf(stderr, "\nSet link up fail.\n");
}
void
dev_set_link_down(portid_t pid)
{
if (rte_eth_dev_set_link_down(pid) < 0)
- printf("\nSet link down fail.\n");
+ fprintf(stderr, "\nSet link down fail.\n");
}
static int
@@ -2353,8 +2343,8 @@ setup_hairpin_queues(portid_t pi, portid_t p_pi, uint16_t cnt_pi)
RTE_PORT_STOPPED) == 0)
printf("Port %d can not be set back "
"to stopped\n", pi);
- printf("Fail to configure port %d hairpin "
- "queues\n", pi);
+ fprintf(stderr, "Fail to configure port %d hairpin queues\n",
+ pi);
/* try to reconfigure queues next time */
port->need_reconfig_queues = 1;
return -1;
@@ -2376,8 +2366,8 @@ setup_hairpin_queues(portid_t pi, portid_t p_pi, uint16_t cnt_pi)
RTE_PORT_STOPPED) == 0)
printf("Port %d can not be set back "
"to stopped\n", pi);
- printf("Fail to configure port %d hairpin "
- "queues\n", pi);
+ fprintf(stderr, "Fail to configure port %d hairpin queues\n",
+ pi);
/* try to reconfigure queues next time */
port->need_reconfig_queues = 1;
return -1;
@@ -2467,8 +2457,9 @@ start_port(portid_t pid)
if (flow_isolate_all) {
int ret = port_flow_isolate(pi, 1);
if (ret) {
- printf("Failed to apply isolated"
- " mode on port %d\n", pi);
+ fprintf(stderr,
+ "Failed to apply isolated mode on port %d\n",
+ pi);
return -1;
}
}
@@ -2477,8 +2468,9 @@ start_port(portid_t pid)
port->socket_id);
if (nb_hairpinq > 0 &&
rte_eth_dev_hairpin_capability_get(pi, &cap)) {
- printf("Port %d doesn't support hairpin "
- "queues\n", pi);
+ fprintf(stderr,
+ "Port %d doesn't support hairpin queues\n",
+ pi);
return -1;
}
/* configure port */
@@ -2490,7 +2482,8 @@ start_port(portid_t pid)
RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
printf("Port %d can not be set back "
"to stopped\n", pi);
- printf("Fail to configure port %d\n", pi);
+ fprintf(stderr, "Fail to configure port %d\n",
+ pi);
/* try to reconfigure port next time */
port->need_reconfig = 1;
return -1;
@@ -2521,8 +2514,9 @@ start_port(portid_t pid)
RTE_PORT_STOPPED) == 0)
printf("Port %d can not be set back "
"to stopped\n", pi);
- printf("Fail to configure port %d tx queues\n",
- pi);
+ fprintf(stderr,
+ "Fail to configure port %d tx queues\n",
+ pi);
/* try to reconfigure queues next time */
port->need_reconfig_queues = 1;
return -1;
@@ -2535,7 +2529,8 @@ start_port(portid_t pid)
mbuf_pool_find
(rxring_numa[pi], 0);
if (mp == NULL) {
- printf("Failed to setup RX queue:"
+ fprintf(stderr,
+ "Failed to setup RX queue:"
"No mempool allocation"
" on the socket %d\n",
rxring_numa[pi]);
@@ -2552,7 +2547,8 @@ start_port(portid_t pid)
mbuf_pool_find
(port->socket_id, 0);
if (mp == NULL) {
- printf("Failed to setup RX queue:"
+ fprintf(stderr,
+ "Failed to setup RX queue:"
"No mempool allocation"
" on the socket %d\n",
port->socket_id);
@@ -2573,8 +2569,9 @@ start_port(portid_t pid)
RTE_PORT_STOPPED) == 0)
printf("Port %d can not be set back "
"to stopped\n", pi);
- printf("Fail to configure port %d rx queues\n",
- pi);
+ fprintf(stderr,
+ "Fail to configure port %d rx queues\n",
+ pi);
/* try to reconfigure queues next time */
port->need_reconfig_queues = 1;
return -1;
@@ -2588,9 +2585,9 @@ start_port(portid_t pid)
diag = rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
NULL, 0);
if (diag < 0)
- printf(
- "Port %d: Failed to disable Ptype parsing\n",
- pi);
+ fprintf(stderr,
+ "Port %d: Failed to disable Ptype parsing\n",
+ pi);
}
p_pi = pi;
@@ -2599,8 +2596,8 @@ start_port(portid_t pid)
/* start port */
diag = rte_eth_dev_start(pi);
if (diag < 0) {
- printf("Fail to start port %d: %s\n", pi,
- rte_strerror(-diag));
+ fprintf(stderr, "Fail to start port %d: %s\n",
+ pi, rte_strerror(-diag));
/* Fail to setup rx queue, return */
if (rte_atomic16_cmpset(&(port->port_status),
@@ -2648,10 +2645,10 @@ start_port(portid_t pid)
continue;
diag = rte_eth_hairpin_bind(pi, peer_pl[j]);
if (diag < 0) {
- printf("Error during binding hairpin"
- " Tx port %u to %u: %s\n",
- pi, peer_pl[j],
- rte_strerror(-diag));
+ fprintf(stderr,
+ "Error during binding hairpin Tx port %u to %u: %s\n",
+ pi, peer_pl[j],
+ rte_strerror(-diag));
return -1;
}
}
@@ -2665,10 +2662,10 @@ start_port(portid_t pid)
continue;
diag = rte_eth_hairpin_bind(peer_pl[j], pi);
if (diag < 0) {
- printf("Error during binding hairpin"
- " Tx port %u to %u: %s\n",
- peer_pl[j], pi,
- rte_strerror(-diag));
+ fprintf(stderr,
+ "Error during binding hairpin Tx port %u to %u: %s\n",
+ peer_pl[j], pi,
+ rte_strerror(-diag));
return -1;
}
}
@@ -2848,7 +2845,8 @@ reset_port(portid_t pid)
port->need_reconfig = 1;
port->need_reconfig_queues = 1;
} else {
- printf("Failed to reset port %d. diag=%d\n", pi, diag);
+ fprintf(stderr, "Failed to reset port %d. diag=%d\n",
+ pi, diag);
}
}
@@ -3103,7 +3101,8 @@ check_all_ports_link_status(uint32_t port_mask)
if (ret < 0) {
all_ports_up = 0;
if (print_flag == 1)
- printf("Port %u link get failed: %s\n",
+ fprintf(stderr,
+ "Port %u link get failed: %s\n",
portid, rte_strerror(-ret));
continue;
}
@@ -3401,7 +3400,7 @@ update_jumbo_frame_offload(portid_t portid)
ret = rte_eth_dev_set_mtu(portid,
port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
if (ret)
- printf("Failed to set MTU to %u for port %u\n",
+ fprintf(stderr, "Failed to set MTU to %u for port %u\n",
port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
portid);
}