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