[dm-devel] [PATCH 1/6] checker: remove useless name check in checker_get func

[Zhiqiang Liu]

On 2020/8/17 23:25, Martin Wilck wrote:
> On Mon, 2020-08-17 at 23:12 +0800, Zhiqiang Liu wrote:
>> On 2020/8/17 16:05, Martin Wilck wrote:
>>> On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
>>>> In checker_get func, input name will be checked before
>>>> calling checker_class_lookup func, in which name will
>>>> also be checked.
>>>>
>>>> Here, we just remove useless input name check in checker_get
>>>> func.
>>>>
>>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
>>>> Signed-off-by: lixiaokeng <lixiaokeng at huawei.com>
>>>> ---
>>>>  libmultipath/checkers.c | 9 ++++-----
>>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> Nack. Better safe than sorry. If you look at the generated
>>> assembly,
>>> you'll see that the compiler optimizes this double check away, so
>>> doesn't inflict runtime overhead, while it makes it easier to
>>> verify
>>> the code.
>>>
>>> I'd ack this if we had a solid convention in the multipath-tools
>>> code
>>> to check parameters always (only) in the callee. But so far we
>>> don't.
>>> I guess if we want to do that, we'd first need to agree on and
>>> implement a common convention for return values, too.
>>>
>> I agree with you.
>> The location of the parameter check is not uniform.
>> Are we dealing with it now? or add it in your TODO list?
> 
> We don't have an official todo list. I tend to handle such "style"
> things incrementally - when I touch some code for some non-trivial
> reason, I also add "style" changes as appropriate (look at "constify"
> changes in recent multipath-tools commits, for example).
> 
> I believe that most functions in multipath-tools already check the
> passed arguments (in the callee), and I believe that this is where it
> should be done. Let's have an eye on this for some time to come.
> Eventually we can audit the entire code to make sure all functions do
> this properly, and once that's done, we can start removing the
> respective checks in callers, at least some.
> 
> Does that make sense?
> 
> Cheers,
> Martin
> 

Thank you for your patience.
Because I am learning and reviewing multipath-tools code,
I'll be happy to push a patch to handle such "style" things.

Regards
Zhiqiang Liu


> 
> 
> 
> 
>>
>>
>>> If checker_class_lookup() was non-static and/or the code was
>>> executed
>>> very often, things would also look different to me. But both is not
>>> the
>>> case.
>>>
>>
>>
>>> Regards,
>>> Martin
>>>
>>>
>>>
>>>> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
>>>> index f7ddd53..ac41d64 100644
>>>> --- a/libmultipath/checkers.c
>>>> +++ b/libmultipath/checkers.c
>>>> @@ -361,11 +361,10 @@ void checker_get(const char *multipath_dir,
>>>> struct checker *dst,
>>>>  	if (!dst)
>>>>  		return;
>>>>
>>>> -	if (name && strlen(name)) {
>>>> -		src = checker_class_lookup(name);
>>>> -		if (!src)
>>>> -			src = add_checker_class(multipath_dir, name);
>>>> -	}
>>>> +	src = checker_class_lookup(name);
>>>> +	if (!src)
>>>> +		src = add_checker_class(multipath_dir, name);
>>>> +
>>>>  	dst->cls = src;
>>>>  	if (!src)
>>>>  		return;
>>>
>>> .
>>>
> 
> 
> 
> .
>