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 B908CA0C40; Fri, 16 Apr 2021 12:44:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A24EC1418AC; Fri, 16 Apr 2021 12:44:27 +0200 (CEST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 1FC0B14176E for ; Fri, 16 Apr 2021 12:44:26 +0200 (CEST) Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FMCSs3BYpzwS3h; Fri, 16 Apr 2021 18:42:05 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS411-HUB.china.huawei.com (10.3.19.211) with Microsoft SMTP Server id 14.3.498.0; Fri, 16 Apr 2021 18:44:22 +0800 To: Kevin Traynor , CC: , , References: <1618046334-39857-1-git-send-email-humin29@huawei.com> <1618447925-12168-1-git-send-email-humin29@huawei.com> <087b2c22-adda-2639-cbdf-2e765f463a91@redhat.com> <3c249770-2c08-cd82-25ef-82429f40bbf3@huawei.com> From: "Min Hu (Connor)" Message-ID: <146942ea-a3f3-a201-9089-63c883737d49@huawei.com> Date: Fri, 16 Apr 2021 18:44:22 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v4] ethdev: add sanity checks in control APIs 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 Sender: "dev" 在 2021/4/16 18:09, Kevin Traynor 写道: > On 16/04/2021 08:00, Min Hu (Connor) wrote: >> Thanks Kevin, >> all is fixed in v6, please review it, thanks. >> Some comments are below. >> >> 在 2021/4/15 20:04, Kevin Traynor 写道: >>> On 15/04/2021 01:52, Min Hu (Connor) wrote: >>>> This patch adds more sanity checks in control path APIs. >>>> >>> >>> Hi Connor, >>> >>> A few general comments, >>> >>> -- >>> Some of the functions have unit tests, you could consider adding unit >>> tests for the new checks. Considering the checks are not subtle and >>> unlikely to be messed up in future, not adding unit tests is not a >>> blocker imho. >>> >>> -- >>> It took me a while to get what you meant with "by NULL". It's usage >>> seems like in "Death by taxes". Perhaps "because NULL ptr" would be a >>> better way to phrase this generically, but I think it is more useful to >>> say what is NULL. >>> >>> e.g. "Failed to convert NULL to string\n" is very generic and would be >>> better as "Failed to convert NULL link to string\n" . ok, still a bit >>> generic but more of a clue. >>> >>> I won't comment on each log message individually but I've added a few >>> suggestions here and there. > > Thanks, I think it looks a lot nicer to read in v6 my completely > subjective biased opinion :-) > >>> -- >>> >>> Did you check the usage of these functions in DPDK, and if the return >>> value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling >>> iterator functions. I'm not sure that having a return check is needed in >>> that case, but there could be other cases where you want to take some >>> different action now. >>> >> As iterator functions are all APIs, they may be used by APP directly. >> I think param check is necessary. > > The point is that it would continue to call the functions even after it > caught this error, so would continue to print error messages. Yes, that > is much better than a seg fault and maybe in this case that is ok. I > will leave it to maintainers to decided. > > I was just wondering if there was additional things similar to this in > DPDK where handling these new errors could now be improved too. I don't > think it has to be a prerequisite for this patch, as this patch is still > an improvement. > Thanks Kevin. Well, for what your metioned, I will try to look for it. If found, I will send another patch or initiate discussions. Thanks. >>> some other comments inlined, >>> >>>> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input") >>>> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables") >>>> Fixes: 0366137722a0 ("ethdev: check for invalid device name") >>>> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model") >>>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership") >>>> Fixes: f8244c6399d9 ("ethdev: increase port id range") > > . >