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 4EB5642367; Wed, 11 Oct 2023 22:51:47 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 249F1400EF; Wed, 11 Oct 2023 22:51:47 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id D7683400D7; Wed, 11 Oct 2023 22:51:45 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 92D35348C; Wed, 11 Oct 2023 22:51:45 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 8704129FF; Wed, 11 Oct 2023 22:51:45 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.5 required=5.0 tests=ALL_TRUSTED,AWL autolearn=disabled version=3.4.6 X-Spam-Score: -1.5 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 7244531B5; Wed, 11 Oct 2023 22:51:44 +0200 (CEST) Message-ID: <2513d49c-2e72-472f-a667-fef90412abe4@lysator.liu.se> Date: Wed, 11 Oct 2023 22:51:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/3] lib: introduce dispatcher library Content-Language: en-US To: David Marchand Cc: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org, Jerin Jacob , techboard@dpdk.org, harry.van.haaren@intel.com, Peter Nilsson , Heng Wang , Naga Harish K S V , Pavan Nikhilesh , Gujjar Abhinandan S , Erik Gabriel Carrillo , Shijith Thotton , Hemant Agrawal , Sachin Saxena , Liang Ma , Peter Mccarthy , Zhirun Yan References: <20230922073825.351453-2-mattias.ronnblom@ericsson.com> <20230928073056.359356-1-mattias.ronnblom@ericsson.com> <20230928073056.359356-2-mattias.ronnblom@ericsson.com> <0c8eaf05-8dc7-4462-a893-2b7019d24fae@lysator.liu.se> <909e0363-365a-4250-85ba-43c8715642a3@lysator.liu.se> From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 On 2023-10-11 16:57, David Marchand wrote: > On Mon, Oct 9, 2023 at 6:50 PM Mattias Rönnblom wrote: > > [snip] > >>>>>> +static int >>>>>> +evd_set_service_runstate(struct rte_dispatcher *dispatcher, int state) >>>>>> +{ >>>>>> + int rc; >>>>>> + >>>>>> + rc = rte_service_component_runstate_set(dispatcher->service_id, >>>>>> + state); >>>>>> + >>>>>> + if (rc != 0) { >>>>>> + RTE_EDEV_LOG_ERR("Unexpected error %d occurred while setting " >>>>>> + "service component run state to %d\n", rc, >>>>>> + state); >>>>>> + RTE_ASSERT(0); >>>>> >>>>> Why not propagating the error to callers? >>>>> >>>>> >>>> >>>> The root cause would be a programming error, hence an assertion is more >>>> appropriate way to deal with the situation. >>> >>> Without building RTE_ENABLE_ASSERT (disabled by default), the code >>> later in this function will still be executed. >>> >> >> If RTE_ASSERT() is not the way to assure a consistent internal library >> state, what is? RTE_VERIFY()? > > The usual way in DPDK is to use RTE_VERIFY or rte_panic with the error message. > There is also libc assert(). > > RTE_ASSERT is more of a debug macro since it is under a build option. > > > But by making the library "panic" on some assertion, I have followup comments: > - what is the point of returning an int for rte_dispatcher_start() / > rte_dispatcher_stop()? > - rte_dispatcher_start() and rte_dispatcher_stop() (doxygen) > documentation needs updating, as they can't return anything but 0. > > Those return vales are purely there for reasons of symmetry, or maybe you can call it API consistent, with Eventdev APIs (e.g., the event ethernet RX adapter). I guess that's less of an issue now, when it's a separate library, but still relevant, since the programmer will be familiar with those APIs. You could also argue that in the future, there may be errors which needs to be signaled to the caller. But that's a weak argument, since we don't know exactly what those would be (and thus they can't be documented), so it's still an API/ABI change, even though the function signature doesn't change. Now I'm leaning toward removing the return value. Please advise. Over.