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

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