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 5AC96A0C3F; Fri, 16 Apr 2021 12:09:54 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 21EC3141CB3; Fri, 16 Apr 2021 12:09:54 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mails.dpdk.org (Postfix) with ESMTP id 53771141CB2 for ; Fri, 16 Apr 2021 12:09:52 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618567791; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mBEFYWrJ+a+V1b+laD7/IJDXnHrBSyGzILr00zz1YAw=; b=Vh1WTCbrr5mxVr2FbXu1T0LN81M78Q3RwDUwfzH9WgRNw2l9E2T/shXF2NRU9CBn18zOfC NnowLddtRzm5Xpt8nOFjmCI9pWsS3FeKc++BddfJaJ6A/ukSPHrTErGgA37KOT/WXW/Rhv fnV5Fde4sGA48iMzDiilHNHgWAYuDH4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-552-VNaMoyByOu2PmlqQ7QltGQ-1; Fri, 16 Apr 2021 06:09:48 -0400 X-MC-Unique: VNaMoyByOu2PmlqQ7QltGQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0E2D483DD20; Fri, 16 Apr 2021 10:09:47 +0000 (UTC) Received: from [10.36.112.36] (ovpn-112-36.ams2.redhat.com [10.36.112.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id CEC4B69FA4; Fri, 16 Apr 2021 10:09:45 +0000 (UTC) To: "Min Hu (Connor)" , dev@dpdk.org Cc: ferruh.yigit@intel.com, thomas@monjalon.net, andrew.rybchenko@oktetlabs.ru 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: Kevin Traynor Message-ID: Date: Fri, 16 Apr 2021 11:09:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <3c249770-2c08-cd82-25ef-82429f40bbf3@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ktraynor@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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. >> 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")