From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C3FBEA2E1B for ; Thu, 5 Sep 2019 09:45:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 622DF1ED82; Thu, 5 Sep 2019 09:45:17 +0200 (CEST) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 0E0AB1EBA0 for ; Thu, 5 Sep 2019 09:45:16 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 61DB82239D; Thu, 5 Sep 2019 03:45:15 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Thu, 05 Sep 2019 03:45:15 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=GVg8KXecuseN6XUs0NI8yd6Y2SI46IKaZboF0HOJ9qU=; b=UUol3JTVg01K NE6S5cgC+yJ1yeRyvJauksN4IFBDFrcTQKZOoTQs8RjV6hXPyl0CRtWyfic3IE1v vqitAtePSXVhJ6kL/xbd5qfWC0t4JgLX+mFiHaE6sx/VPG/zuq1QLdPBwA+8mfy2 6U439d0lDZVGmK8MH8E+tRshGrv9+q4= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=GVg8KXecuseN6XUs0NI8yd6Y2SI46IKaZboF0HOJ9 qU=; b=RSkgrzsihNNqjCgu+Pxf2XhoT0QuX/FpTQazzzhj+DPnZTvxg5tcLjozG ZdfA8KCgf+JvEgjPw3YNTmlXlZv84ftFRqT2JJtIQSLqvFEtjxyLSfe+CK1yRsFc P+xZBFzHfM78HI5LQP5OUeXvExGpXUuIIFwRXLEveXW5mGXJpP4y3DY/PlXlP//f vTS+57Yqo1StVtofZCDZuW4sTLvmB0+3c6sEekv58Ijm6FZNwDU5lspRT9czqdhR DWnwx+to/SBaifgc4FMHyi1oHEz5Lb6ny2VaWE8NOYP5wFeIGBvQ/5yy+BYck3fy UQClakRlm+7EvF71QR1L1NcR38qbQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduvddrudejiedguddujecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ffohhmrghinhepughpughkrdhorhhgnecukfhppeejjedrudefgedrvddtfedrudekgeen ucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth enucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 0A0FA80062; Thu, 5 Sep 2019 03:45:13 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: Ferruh Yigit , David Marchand , dev@dpdk.org, Olivier Matz Date: Thu, 05 Sep 2019 09:45:12 +0200 Message-ID: <195959620.aLVmYajP68@xps> In-Reply-To: <5b3f331c-4011-63bf-3f38-44b160a6be66@solarflare.com> References: <1566214919-32250-1-git-send-email-david.marchand@redhat.com> <4083300.tcgOVcdeWQ@xps> <5b3f331c-4011-63bf-3f38-44b160a6be66@solarflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH 02/11] log: define logtype register wrapper for drivers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" 05/09/2019 08:29, Andrew Rybchenko: > On 9/4/19 11:44 PM, Thomas Monjalon wrote: > > 04/09/2019 21:58, Andrew Rybchenko: > >> On September 4, 2019 22:42:12 Thomas Monjalon wrote: > >> > >>> 04/09/2019 21:21, Andrew Rybchenko: > >>>> On 9/4/19 8:45 PM, Thomas Monjalon wrote: > >>>>> 03/09/2019 10:47, Ferruh Yigit: > >>>>>> On 9/3/2019 9:06 AM, David Marchand wrote: > >>>>>>> On Mon, Sep 2, 2019 at 4:29 PM Ferruh Yigit wrote: > >>>>>>>> On 8/19/2019 12:41 PM, David Marchand wrote: > >>>>>>>>> The function rte_log_register_type_and_pick_level() fills a gap for > >>>>>>>>> dynamically loaded code (especially drivers) who would not pick up > >>>>>>>>> the log level passed at startup. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> Let's promote it to stable and export it for use by drivers via > >>>>>>>>> a wrapper. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> Signed-off-by: David Marchand > >>>>>>>>> --- > >>>>> [...] > >>>>>>>>> /** > >>>>>>>>> - * @warning > >>>>>>>>> - * @b EXPERIMENTAL: this API may change without prior notice > >>>>>>>>> - * > >>>>>>>>> * Register a dynamic log type and try to pick its level from EAL options > >>>>> [...] > >>>>>>>>> -__rte_experimental > >>>>>>>>> int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def); > >>>>>>>> +1 to remove experimental from the API. > >>>>> I am not sure about this function API. > >>>>> Why we combined register and level setting in one function? > >>>> See [1] > >>>> > >>>> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026 > >>> Sorry, it does not explain why we mix both operations in one function. > >> Exactly to have one function instead of repeating two function calls > >> everywhere. Log level should be set when log type is registered. Yes, it is > >> possible to factor out a function just to pick log level, but I'm not sure > >> it makes sense separately. > >> > >> In fact may be it makes sense to substitute just register with this one > >> (I.e. remove simple register from piblic API and do not highlight that > >> level is picked up in function name). > > Given that we use it in a macro, we could keep two separate functions > > for logtype register and minimum log level (note that "minimum" is > > currently a missing word in these functions). > > Anyway, we will always use the single macro in our libraries. > > I don't understand why "minimum" is missing. It assigns level specified > for the pattern if it matches, or use level_def if no match. No level > comparison, last match wins. If "minimum" refers to minimum logged > level, it matches rte_log_set_level() which has no "minimum" as well > in neither name nor description. Moreover, EMERG is smaller than > DEBUG and if we treat log levels as numbers, it sets maximum level > which is logged. Yes it is maximum. I mean even in rte_log_set_level() we don't have such word. It is a detail. > There are two usages right now without a macro, but it is not that > important. What I'm trying to say is that rte_log_register() plus setting > default after register is too error prone. It is the source of the bug > which we tried to workaround introducing the function. > > If user sets log level (using eal command-line options), no special > efforts should be required to assign it on register. So, I think register > should always do it. The function name is not ideal since it is highlights > details which are not really interesting. It is logging support details that > log levels may be set before log types registration. > > if we accept DPDK-wide default, it makes level_def parameter > unnecessary and the functionality to pick level may be simply > moved to rte_log_register() (using helper internal function) and > I see no good reasons why we really need type-specific defaults. I agree it can be merged in rte_log_register().