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 0C4C04233F; Mon, 9 Oct 2023 19:40:15 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D8071402B1; Mon, 9 Oct 2023 19:40:14 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 43C404026B; Mon, 9 Oct 2023 19:40:14 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id F15A926438; Mon, 9 Oct 2023 19:40:13 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id E598926250; Mon, 9 Oct 2023 19:40:13 +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 10DF4261CF; Mon, 9 Oct 2023 19:40:10 +0200 (CEST) Message-ID: Date: Mon, 9 Oct 2023 19:40:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/3] lib: introduce dispatcher library Content-Language: en-US To: Thomas Monjalon Cc: techboard@dpdk.org, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org, Jerin Jacob , 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 , David Marchand References: <20230922073825.351453-2-mattias.ronnblom@ericsson.com> <0c8eaf05-8dc7-4462-a893-2b7019d24fae@lysator.liu.se> <13795063.RDIVbhacDa@thomas> From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <13795063.RDIVbhacDa@thomas> 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-06 11:03, Thomas Monjalon wrote: > 06/10/2023 10:46, David Marchand: >> On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom wrote: >>>>> +static int >>>>> +evd_lookup_handler_idx(struct rte_dispatcher_lcore *lcore, >>>>> + const struct rte_event *event) >>>> >>>> Wrt DPDK coding tyle, indent is a single tab. >>>> Adding an extra tab is recommended when continuing control statements >>>> like if()/for()/.. >>>> >>> >>> Sure, but I don't understand why you mention this. >> >> I wanted to remind the DPDK coding style which I try to more closely >> enforce for new code. >> indent is off in this file (especially for function prototypes with >> multiple tabs used). >> >>> >>>> On the other hand, max accepted length for a line is 100 columns. >>>> >>>> Wdyt of a single line for this specific case? >>> >>> >>> Are you asking why the evd_lookup_handler_idx() function prototype is >>> not a single line? >>> >>> It would make it long, that's why. Even if 100 wide lines are allowed, >>> it doesn't means the author is forced to use such long lines? >> >> I find it more readable. >> If you want to stick to 80 columns, please comply with a single tab for indent. > > I think this is a case of continuation line, so it should be 2 tabs. > We can make it clear in the doc. > > >>>>> +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. > > Don't forget you are writing a library, so you shouldn't stop runtime > because of an error. It is always better to return errors. > Assert should be used only for debugging, that's why it is disabled by default. > > > A breach of the API contract should always be met with an assertion - library or not. That is the most helpful thing you can do, so that one fails early and programmer can go fix the bug. The use of EINVAL and attempts to return error in the face of API violations is a bad practice, in my opinion. What could the program possible do, if it learns it's using a function the wrong way? If the two memory areas in memmove() overlap, should there be a error code, so that caller can retry but "please not with overlapping memory regions this time". I know that the EINVAL pattern is wide-spread practice in DPDK, at the cost of performance and complexity, I would argue. I guess this practice originates from the kernel/libc tradition of validating system calls (a place where this can be actually be done in a reliable manner, unlike in normal user code).