Re: [dpdk-dev] [PATCH] app/testpmd: send failure logs to stderr

[Ferruh Yigit <ferruh.yigit@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>
>>>>>
>>>
>