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 8F54EA046B for ; Wed, 26 Jun 2019 15:20:12 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5C6832AA0; Wed, 26 Jun 2019 15:20:12 +0200 (CEST) Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id 1E19F1E25 for ; Wed, 26 Jun 2019 15:20:11 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id A29A6221D8; Wed, 26 Jun 2019 09:20:10 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 26 Jun 2019 09:20:10 -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=r3spswIaBhU+noinOJEGu1Gl+B6k2c3tpNz3/0eVDqQ=; b=MYMFm1ixy6jG oyFtC7uWE07GLJd/xl3OQvVq5/FRY2zza7G/oqazyXyf6BlNQQi1KS65oyumZLC9 1noviS6n3FlP4VzQ+c5eX1gG/Qkcqbkq3ANgRr/SonsT9UTlqp93zDc+WdUZqbt3 rYS+CwRxYVzWTWpUpvRd8SxTjMnp8AQ= 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=r3spswIaBhU+noinOJEGu1Gl+B6k2c3tpNz3/0eVD qQ=; b=TpuOcREW+UfPGJzZfJL+P0zsWVwADT7eILNKWb5IKezyMaqUuYD2xuTsI 9CYdx8N0QSLi1FXEqziBDgd+kEiseg5ZHvKCQV86uyR/aCfu7ypzBPSi9YgutOhT eTAO/2F1pcZMDHjcP2/YNLs0tNmoGyloxhsnByQaIlwbtmR8XUJtSW15awNS+5lC 3VO+evrvarTz5XQEmTOCUi/Rn40ATI89+LBbA0DqODvqnvmfJ30KmeXME45hfxS0 ARk4ZYlOY/WY+dc43XwDcNCcXtlzb1ssB+7AvcICJ8M5X0mTFON7zhQSx6IZduOr Iz1ycr9O7RdfFxoN7H/dctJUiNanQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduvddrudeigdeigecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epkedurddukeehrdduieefrdekfeenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhm rghssehmohhnjhgrlhhonhdrnhgvthenucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from xps.localnet (83.163.185.81.rev.sfr.net [81.185.163.83]) by mail.messagingengine.com (Postfix) with ESMTPA id E0D7A8006A; Wed, 26 Jun 2019 09:20:08 -0400 (EDT) From: Thomas Monjalon To: "Burakov, Anatoly" Cc: Bruce Richardson , David Marchand , dev Date: Wed, 26 Jun 2019 15:20:05 +0200 Message-ID: <3895626.Y0NUr5ymUO@xps> In-Reply-To: References: <20190626104056.26829-1-thomas@monjalon.net> <20190626123623.GB862@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] eal/linux: fix return after alarm registration failure 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" 26/06/2019 14:52, Burakov, Anatoly: > On 26-Jun-19 1:36 PM, Bruce Richardson wrote: > > On Wed, Jun 26, 2019 at 01:55:53PM +0200, Thomas Monjalon wrote: > >> 26/06/2019 13:43, Burakov, Anatoly: > >>> On 26-Jun-19 12:39 PM, David Marchand wrote: > >>>> On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon wrote: > >>>> > >>>>> 26/06/2019 13:20, David Marchand: > >>>>>> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon > >>>>>> wrote: > >>>>>> > >>>>>>> When adding an alarm, if an error happen when registering > >>>>>>> the common alarm callback, it is not considered as a major failure. > >>>>>>> The alarm is then inserted in the list. > >>>>>>> However it was returning an error code after inserting the alarm. > >>>>>>> > >>>>>>> The error code is reset to 0 so the behaviour and the return code > >>>>>>> are consistent. > >>>>>>> Other return code related lines are cleaned up for easier > >>>>> understanding. > >>>>>>> > >>>>> [...] > >>>>>>> --- a/lib/librte_eal/linux/eal/eal_alarm.c > >>>>>>> +++ b/lib/librte_eal/linux/eal/eal_alarm.c > >>>>>>> if (!handler_registered) { > >>>>>>> - ret |= rte_intr_callback_register(&intr_handle, > >>>>>>> + ret = rte_intr_callback_register(&intr_handle, > >>>>>>> eal_alarm_callback, NULL); > >>>>>>> - handler_registered = (ret == 0) ? 1 : 0; > >>>>>>> + if (ret == 0) > >>>>>>> + handler_registered = 1; > >>>>>>> + else > >>>>>>> + /* not fatal, callback can be registered later > >>>>> */ > >>>>>>> + ret = 0; > >>>>>>> } > >>>>>> > >>>>>> Well, then it means that you don't want to touch ret at all. > >>>>>> How about: > >>>>>> if (rte_intr_callback_register(&intr_handle, > >>>>>> eal_alarm_callback, NULL) == 0) > >>>>>> handler_registered = 1; > >>>>>> > >>>>>> ? > >>>>> > >>>>> Too much simple :) > >>>>> > >>>>> I think we try to avoid calling a function in a "if" > >>>>> per coding style. > >>>>> And my proposal has the benefit of offering a comment > >>>>> about the non-fatal error. > >>>>> > >>>> > >>>> /* not fatal, callback can be registered later */ > >>>> if (rte_intr_callback_register(&intr_handle, > >>>> eal_alarm_callback, NULL) == 0) > >>>> handler_registered = 1; > >>>> > >>> > >>> I prefer the original. It's more explicit and conveys the intention > >>> better. Did i break the tie? :) > >> > >> I was going to send a v2 with David's suggestion. > >> Now I'm confused. > >> > > I always tend to prefer shorter versions, so +1 for v2 (does that make it a > > v3? :-) ) > > > > /Bruce > > > > OK, but then the suggested comment needs to be fixed. It makes it seem > like registering the handler is the "non fatal" part. Perhaps something > like: > > /* failed register is not a fatal error - callback can be registered > later */ Of course! I had prepared this: /* registration can fail, callback can be registered later */