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 4F0AC432C2; Tue, 7 Nov 2023 08:18:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E901C402BE; Tue, 7 Nov 2023 08:18:16 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 34765402BC for ; Tue, 7 Nov 2023 08:18:16 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 8548C5E; Tue, 7 Nov 2023 10:18:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 8548C5E DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1699341495; bh=fDuOW5Jp2jkFYV+bm8uEfv8MSguYQisQjsopuxxuJZc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=qdmtWo1fBR8CAgsx1SM/HkPp7See5c93f2Kz+8BeZVWH+yPNJJEfeVpDh+X3jdclT UPyofafVZF8s/LoopSui9H1vyG6tQjQISfmrswzDRVNBkPCXatw8rF4lIbPjLA1jE0 K84+nECs2tuEledwCZhYQjB95RsDojSqR0WxfHYE= Message-ID: <52756c58-7081-4393-9e3d-88a96e5a5c4b@oktetlabs.ru> Date: Tue, 7 Nov 2023 10:18:15 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/5] net/sfc: use new API to parse kvargs Content-Language: en-US To: fengchengwen , thomas@monjalon.net, ferruh.yigit@amd.com Cc: dev@dpdk.org, stephen@networkplumber.org, Vijay Kumar Srivastava References: <20230314124813.39521-1-fengchengwen@huawei.com> <20231106073125.55280-1-fengchengwen@huawei.com> <20231106073125.55280-3-fengchengwen@huawei.com> <4afbee3e-226c-4c1d-9a87-0a6198be5005@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 11/6/23 15:29, fengchengwen wrote: > Hi Andrew, > > On 2023/11/6 18:28, Andrew Rybchenko wrote: >> On 11/6/23 10:31, Chengwen Feng wrote: >>> The sfc_kvargs_process() and sfc_efx_dev_class_get() function could >>> handle both key=value and only-key, so they should use >>> rte_kvargs_process_opt() instead of rte_kvargs_process() to parse. >>> >>> Signed-off-by: Chengwen Feng >>> --- >>>   drivers/common/sfc_efx/sfc_efx.c | 4 ++-- >>>   drivers/net/sfc/sfc_kvargs.c     | 2 +- >>>   2 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/common/sfc_efx/sfc_efx.c b/drivers/common/sfc_efx/sfc_efx.c >>> index 2dc5545760..3ebac909f1 100644 >>> --- a/drivers/common/sfc_efx/sfc_efx.c >>> +++ b/drivers/common/sfc_efx/sfc_efx.c >>> @@ -52,8 +52,8 @@ sfc_efx_dev_class_get(struct rte_devargs *devargs) >>>           return dev_class; >>>         if (rte_kvargs_count(kvargs, RTE_DEVARGS_KEY_CLASS) != 0) { >>> -        rte_kvargs_process(kvargs, RTE_DEVARGS_KEY_CLASS, >>> -                   sfc_efx_kvarg_dev_class_handler, &dev_class); >>> +        rte_kvargs_process_opt(kvargs, RTE_DEVARGS_KEY_CLASS, >>> +                       sfc_efx_kvarg_dev_class_handler, &dev_class); >> >> LGTM from code point of view, but I'm not sure that I understand the >> idea behind handling NULL value in sfc_efx_kvarg_dev_class_handler(). >> >> Cc: Vijay >> >>>       } >>>         rte_kvargs_free(kvargs); >>> diff --git a/drivers/net/sfc/sfc_kvargs.c b/drivers/net/sfc/sfc_kvargs.c >>> index 783cb43ae6..24bb896179 100644 >>> --- a/drivers/net/sfc/sfc_kvargs.c >>> +++ b/drivers/net/sfc/sfc_kvargs.c >>> @@ -70,7 +70,7 @@ sfc_kvargs_process(struct sfc_adapter *sa, const char *key_match, >>>       if (sa->kvargs == NULL) >>>           return 0; >>>   -    return -rte_kvargs_process(sa->kvargs, key_match, handler, opaque_arg); >>> +    return -rte_kvargs_process_opt(sa->kvargs, key_match, handler, opaque_arg); >> >> It looks wrong to me since many handlers do not handle NULL string gracefully. As I understand some handlers where fixed to avoid crash >> and correct fix would be to keep  rte_kvargs_process() and remove >> unnecessary checks for NULL string value. > > The scope is large, I suggest creates a new patchset later which remove unnecessary checks for NULL string value. I just want to highlight that it will not make this patch correct. So, Nack > >> >>>   } >>>     int >> >> .