Re: [PATCH] nvme-fabrics: open code __nvmf_host_find()

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 6/3/23 17:35, Max Gurtovoy wrote:
> Hi,
>
> On 02/06/2023 9:47, Chaitanya Kulkarni wrote:
>> There is no point in maintaining a separate funciton __nvmf_host_find()
>
> typo *function
>
>> that has only one caller nvmf_host_add() especially when caller and
>> callee both are small enough to merge.
>>
>> Due to this we are actually repeating the error handling code in both
>> callee and caller for no reason that can be avoided, but instead we have
>> to read both function to establish the correctness along with additional
>> lockdep warning check due to involved locking.
>>
>> Just open code __nvmf_host_find() in nvme_host_alloc() with appropriate
>> comment that removes repeated error checks in the callee/caller and
>> lockdep check that is needed for the nvmf_hosts_mutex involvement,
>> diffstats :-
>
> The above 2 sentences are redundant IMO.
> There is no error handling in the callee so it can't be repeated. We 
> just return error in the callee.

and that is error handling, without error conditions we would
only returning either valid host (that is non NULL) or NULL in
absence of host in the list. That -EINVAL return is the reason
we need separate check with IS_ERR in the caller ...

-    if (IS_ERR(host)) {
-        goto out_unlock;
-    } else if (host) {
-        kref_get(&host->ref);
-        goto out_unlock;

> The first sentence is good enough to justify this patch.
>
> Lets have instead:
>
> "Merge its code with the only caller nvmf_host_add() since both are 
> small enough.
> The lockdep check in __nvmf_host_find() after the merge becomes 
> redundant so we can remove it too."
>
>

I don't understand what you are saying, provide a complete commit
log that can be applied verbatim to this patch.

-ck