On 7/17/2014 23:13, J. Bruce Fields wrote:
> On Thu, Jul 17, 2014 at 09:24:13PM +0800, Kinglong Mee wrote:
>> On 7/17/2014 06:05, J. Bruce Fields wrote:
>>> On Sat, Jul 12, 2014 at 10:02:17PM +0800, Kinglong Mee wrote:
>>>> On 7/12/2014 05:16, J. Bruce Fields wrote:
>>>>> The local variable "ses" will be left NULL here in the case we fail to
>>>>> find a connection. Spotted by a coverity scan.
>>>>>
>>>>> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>>>>> index 2c73cae9899d..fe22cd5c42d3 100644
>>>>> --- a/fs/nfsd/nfs4callback.c
>>>>> +++ b/fs/nfsd/nfs4callback.c
>>>>> @@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
>>>>> }
>>>>> spin_unlock(&clp->cl_lock);
>>>>>
>>>>> + if (!c)
>>>>> + goto out_no_connection;
>>>>
>>>> Setting err to -EINVAL maybe better.
>>>> Otherwise, nfsd4_mark_cb_down will be called with err == 0.
>>>>
>>>>> err = setup_callback_client(clp, &conn, ses);
>>>>
>>>> setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL.
>>>
>>> Thanks, yes, after looking over this carefully I don't believe we can
>>> call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so
>>> this is just a false positive from coverity.
>>
>> ses and conn->cb_xprt will be set in the same condition before
>> calling setup_callback_client,
>>
>> 996 c = __nfsd4_find_backchannel(clp);
>> 997 if (c) {
>> 998 svc_xprt_get(c->cn_xprt);
>> 999 conn.cb_xprt = c->cn_xprt;
>> 1000 ses = c->cn_session;
>> 1001 }
>> 1002 spin_unlock(&clp->cl_lock);
>> 1003
>> 1004 err = setup_callback_client(clp, &conn, ses);
>>
>> ses and conn->cb_xprt will be NULL or non-NULL in the same time,
>> so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear.
>
> Agreed but you also need to check what happens in the case where c is
> NULL.
>
> This is much more confusing than necessary. But I'm inclined to hold
> off on any cleanup here while jlayton's patches are still pending.
Got it.
I also have some cleanup, will send after jlayton's patches merged.
thanks,
Kinglong Mee
>
> --b.
>
>>
>> thanks,
>> Kinglong Mee
>>
>>>>> - if (err) {
>>>>> - nfsd4_mark_cb_down(clp, err);
>>>>> - return;
>>>>> - }
>>>>> + if (err)
>>>>> + goto out_no_connection;
>>>>> /* Yay, the callback channel's back! Restart any callbacks: */
>>>>> list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
>>>>> run_nfsd4_cb(cb);
>>>>> + return;
>>>>> +out_no_connection:
>>>>> + nfsd4_mark_cb_down(clp, err);
>>>>> + return;
>>>>> }
>>>>>
>>>>> static void nfsd4_do_callback_rpc(struct work_struct *w)
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
