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

[Zhiqiang Liu]

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?


> 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;
> 
> 
> .
>