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@xxxxxxxxxx>
>> Signed-off-by: lixiaokeng <lixiaokeng@xxxxxxxxxx>
>> ---
>> 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;
>
>
> .
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
