DPDK patches and discussions
 help / color / mirror / Atom feed
From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	"Li, Xiaoyun" <xiaoyun.li@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: send failure logs to stderr
Date: Tue, 15 Jun 2021 11:14:34 +0300	[thread overview]
Message-ID: <44643b14-0d89-1f8b-e60f-1bc13dfa3cd2@oktetlabs.ru> (raw)
In-Reply-To: <11e9bea3-0d44-f55e-51e2-a6afc4b4c1a3@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>
>>>>
>>


  reply	other threads:[~2021-06-15  8:14 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-27 16:24 Andrew Rybchenko
2021-06-11  2:06 ` Li, Xiaoyun
2021-06-11  9:19   ` Andrew Rybchenko
2021-06-11 10:35     ` Ferruh Yigit
2021-06-11 13:21       ` Bruce Richardson
2021-06-14 16:47         ` Andrew Rybchenko
2021-06-14 16:56       ` Andrew Rybchenko
2021-06-14 17:49         ` Singh, Aman Deep
2021-06-15  7:59         ` Ferruh Yigit
2021-06-15  8:14           ` Andrew Rybchenko [this message]
2021-06-15  8:52             ` Ferruh Yigit
2021-06-15  9:00               ` Andrew Rybchenko
2021-06-15  9:53                 ` Ferruh Yigit
2021-06-16 16:32 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
2021-06-17  3:29   ` Li, Xiaoyun
2021-06-17 14:21     ` Andrew Rybchenko
2021-06-17 14:20 ` [dpdk-dev] [PATCH v3] " Andrew Rybchenko
2021-06-18  8:32   ` Li, Xiaoyun
2021-06-28 11:05     ` Andrew Rybchenko
2021-07-24 13:14     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=44643b14-0d89-1f8b-e60f-1bc13dfa3cd2@oktetlabs.ru \
    --to=andrew.rybchenko@oktetlabs.ru \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=xiaoyun.li@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).