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 5385F42825; Thu, 23 Mar 2023 13:51:11 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 38D5A4021E; Thu, 23 Mar 2023 13:51:11 +0100 (CET) Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by mails.dpdk.org (Postfix) with ESMTP id BD3E54021D for ; Thu, 23 Mar 2023 13:51:10 +0100 (CET) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id B3A0C5C00D2; Thu, 23 Mar 2023 08:51:07 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 23 Mar 2023 08:51:07 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to; s=fm2; t= 1679575867; x=1679662267; bh=sI0wvsQ7F1Lf3vFi0HbP8wVmPVd4PAv3unX cBqNxpSQ=; b=1EKVkskifQ5wpIQrlOkLAo6jt0d1ALzslbCQtbsoUss3fjDHzYw 0Epoo0nbY+1UY8/mzv8Y00KuiarXptSQviVLDQLVZJbL6/fRJlFzQ6JwMX/ccKdU MREIsG/kAYDNvwYL17ojwUZtrAmXG/QI0tmNplyrcyBmh/5X4b7Bzq/j6p3X12xg o9GphxWvL94eCD6nTOblMr8ytjxi4OubUCXISk13d8Cd64NR8jV3tT7IbXc4osRz 6/qpCR7EJ+/RN7ogkjQkV2hKtQh9ArkxlbgNPTAIptORTnQc7Sjf19SynprcmErC 6JntSsPcbKNJjl1COKdM9Oy3s0/ceMGTwAg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t= 1679575867; x=1679662267; bh=sI0wvsQ7F1Lf3vFi0HbP8wVmPVd4PAv3unX cBqNxpSQ=; b=TWjfCPdNTy/85j3PNf95vQ4WQCcLvEmyiry/nd0pA2WJXVrnjHS dQLjx6I3TkkN11XKWP2P682bbVoXINjBNRV4N+I4dNv9d97HL6Iv8Dbc/CLOZ0or NBBgdXfWy9tTxjmlHetLDJxvYFlvaCn/C47yjbA/8fpNQR6SHMhTWTe9FtyPieNi CPq6sr9tKR2Uh2d5zbus5OT96K+LyxhnHK84t0aZe0lciqFH+w8yL5k5sg9ypzze fb4LkKsr/X49VvkBOr+PWEfg45PxDU1GlVUQmMD4FCzWCAcruolrTfy8Dv/MuORr IJRdeUcVS28ejvMEhsdKLz9ih8K47r7JQEg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrvdeggedggeegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedtjeeiieefhedtfffgvdelteeufeefheeujefgueetfedttdei kefgkeduhedtgfenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 23 Mar 2023 08:51:06 -0400 (EDT) From: Thomas Monjalon To: Olivier Matz , Ferruh Yigit , fengchengwen Cc: dev@dpdk.org, David Marchand Subject: Re: [PATCH 0/5] fix segment fault when parse args Date: Thu, 23 Mar 2023 13:51:05 +0100 Message-ID: <2138436.GUh0CODmnK@thomas> In-Reply-To: <393771c8-c98b-5796-cab1-7ce267b33043@huawei.com> References: <20230314124813.39521-1-fengchengwen@huawei.com> <5169242.6fTUFtlzNn@thomas> <393771c8-c98b-5796-cab1-7ce267b33043@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 23/03/2023 12:58, fengchengwen: > 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 ? It is a bit too late to take a decision in 23.03 cycle. Let's continue this discussion. We can either have some fixes in 23.07 or have an ABI breaking change in 23.11.