Re: [dpdk-dev] [PATCH v4] ethdev: add sanity checks in control APIs

["Min Hu (Connor)" <humin29@huawei.com>]

在 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")
> 
> .
>