From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C34C3A0C47; Tue, 15 Jun 2021 11:53:18 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4F3A14067A; Tue, 15 Jun 2021 11:53:18 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id A92A840140 for ; Tue, 15 Jun 2021 11:53:16 +0200 (CEST) IronPort-SDR: Y6edvTm97pTXs0Z1rYUDe7wsaufdu4YPelQVH0Z1NrOR1sIvJSP3jq2XpPJOA00BzfYbCJZ57x 8HkcN/YBn1lQ== X-IronPort-AV: E=McAfee;i="6200,9189,10015"; a="202934559" X-IronPort-AV: E=Sophos;i="5.83,275,1616482800"; d="scan'208";a="202934559" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jun 2021 02:53:15 -0700 IronPort-SDR: +98bNXB8FD9NM9q/MQQ9YjW/0qSQUDm2Vouqez0RlHaaI6JDroDO/+1xlkjbYnK5AeWtMTtm6A xH4cyaxIpkxw== X-IronPort-AV: E=Sophos;i="5.83,275,1616482800"; d="scan'208";a="421072677" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.200.187]) ([10.213.200.187]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jun 2021 02:53:13 -0700 To: Andrew Rybchenko , "Li, Xiaoyun" Cc: "dev@dpdk.org" , Bruce Richardson References: <20210527162452.1568351-1-andrew.rybchenko@oktetlabs.ru> <1f419fbb-b951-1a84-3329-97701c32c956@oktetlabs.ru> <87dd3fd0-9dab-8079-d135-50d966d6a5cd@intel.com> <2876ce81-7dfc-5e02-2f4a-9861dab1f877@oktetlabs.ru> <11e9bea3-0d44-f55e-51e2-a6afc4b4c1a3@intel.com> <44643b14-0d89-1f8b-e60f-1bc13dfa3cd2@oktetlabs.ru> <2453ce9c-82c0-cb1c-9402-3e460d40c710@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <08105d43-6ee7-9563-2d3a-99773d0ae65f@intel.com> Date: Tue, 15 Jun 2021 10:53:09 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: send failure logs to stderr X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>>>>>>> Sent: Friday, May 28, 2021 00:25 >>>>>>>> To: Li, Xiaoyun >>>>>>>> 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 >>>>>>>> --- >>>>>>>> >>>>>>>> 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 >