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.rybche...@oktetlabs.ru> >>>>>>>> Sent: Friday, May 28, 2021 00:25 >>>>>>>> To: Li, Xiaoyun <xiaoyun...@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.rybche...@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...@intel.com> >
