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 0E76D42825; Thu, 23 Mar 2023 12:58:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E2DED4021E; Thu, 23 Mar 2023 12:58:56 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id 7D67E4021D for ; Thu, 23 Mar 2023 12:58:54 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.54]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4Pj3l939Q2zKnDY; Thu, 23 Mar 2023 19:58:29 +0800 (CST) Received: from [10.67.100.224] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Thu, 23 Mar 2023 19:58:52 +0800 Subject: Re: [PATCH 0/5] fix segment fault when parse args To: Thomas Monjalon , Olivier Matz , Ferruh Yigit CC: , David Marchand References: <20230314124813.39521-1-fengchengwen@huawei.com> <6e6b0618-3f36-1832-19a2-9dcadd62de9b@amd.com> <5169242.6fTUFtlzNn@thomas> From: fengchengwen Message-ID: <393771c8-c98b-5796-cab1-7ce267b33043@huawei.com> Date: Thu, 23 Mar 2023 19:58:52 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <5169242.6fTUFtlzNn@thomas> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected 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/3/22 21:49, Thomas Monjalon wrote: > 22/03/2023 09:53, Ferruh Yigit: >> On 3/22/2023 1:15 AM, fengchengwen wrote: >>> On 2023/3/21 21:50, Ferruh Yigit wrote: >>>> On 3/17/2023 2:43 AM, fengchengwen wrote: >>>>> On 2023/3/17 2:18, Ferruh Yigit wrote: >>>>>> On 3/14/2023 12:48 PM, Chengwen Feng wrote: >>>>>>> The rte_kvargs_process() was used to parse KV pairs, it also supports >>>>>>> to parse 'only keys' (e.g. socket_id) type. And the callback function >>>>>>> parameter 'value' is NULL when parsed 'only keys'. >>>>>>> >>>>>>> It may leads to segment fault when parse args with 'only key', this >>>>>>> patchset fixes rest of them. >>>>>>> >>>>>>> Chengwen Feng (5): >>>>>>> app/pdump: fix segment fault when parse args >>>>>>> net/memif: fix segment fault when parse devargs >>>>>>> net/pcap: fix segment fault when parse devargs >>>>>>> net/ring: fix segment fault when parse devargs >>>>>>> net/sfc: fix segment fault when parse devargs >>>>>> >>>>>> Hi Chengwen, >>>>>> >>>>>> Did you scan all `rte_kvargs_process()` instances? >>>>> >>>>> No, I was just looking at the modules I was concerned about. >>>>> I looked at it briefly, and some modules had the same problem. >>>>> >>>>>> >>>>>> >>>>>> And if there would be a way to tell kvargs that a value is expected (or >>>>>> not) this checks could be done in kvargs layer, I think this also can be >>>>>> to look at. >>>>> >>>>> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. >>>>> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene. >>>>> But it also break the API's behavior. >>>>> >>>> >>>> What about having a new API, like `rte_kvargs_process_extended()`, >>>> >>>> That gets an additional flag as parameter, which may have values like >>>> following to indicate if key expects a value or not: >>>> ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' >>>> ARG_WITH_VALUE --> "key=value" >>>> ARG_NO_VALUE --> 'key' >>>> >>>> Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as >>>> `rte_kvargs_process()`. >>>> >>>> This way instead of adding checks, relevant usage can be replaced by >>>> `rte_kvargs_process_extended()`, this requires similar amount of change >>>> but code will be more clean I think. >>>> >>>> Do you think does this work? >>> >>> Yes, it can work. >>> >>> But I think the introduction of new API adds some complexity. >>> And a good API definition could more simpler. >>> >> >> Other option is changing existing API, but that may be widely used and >> changing it impacts applications, I don't think it worth. > > I've planned a change in kvargs API 5 years ago and never did it: >>>From doc/guides/rel_notes/deprecation.rst: > " > * kvargs: The function ``rte_kvargs_process`` will get a new parameter > for returning key match count. It will ease handling of no-match case. > " I think it's okay to add extra parameter for rte_kvargs_process. But it will break ABI. Also I notice patchset was deferred in patchwork. Does it mean that the new version can't accept until the 23.11 release cycle ? > >> Of course we can live with as it is and add checks to the callback >> functions, although I still believe a new 'process()' API is better idea. > > > > . >