On 12/14/07 5:19 PM, "J. Bruce Fields" <[EMAIL PROTECTED]> wrote:
> On Tue, Dec 11, 2007 at 05:33:00PM -0600, Tom Tucker wrote:
>>
>> Move the code that adds a transport instance to the sv_tempsocks and
>> sv_permsocks lists out of the transport specific functions and into core
>> logic.
>>
>> Signed-off-by: Tom Tucker <[EMAIL PROTECTED]>
>> ---
>>
>> net/sunrpc/svc_xprt.c | 9 ++++++++-
>> net/sunrpc/svcsock.c | 39 +++++++++++++++++++--------------------
>> 2 files changed, 27 insertions(+), 21 deletions(-)
>>
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index d0cbfe0..924df63 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -145,8 +145,15 @@ int svc_create_xprt(struct svc_serv *serv, char
>> *xprt_name, unsigned short port,
>> if (IS_ERR(newxprt)) {
>> module_put(xcl->xcl_owner);
>> ret = PTR_ERR(newxprt);
>> - } else
>> + } else {
>> + clear_bit(XPT_TEMP,
>> + &newxprt->xpt_flags);
>> + spin_lock_bh(&serv->sv_lock);
>> + list_add(&newxprt->xpt_list,
>> + &serv->sv_permsocks);
>> + spin_unlock_bh(&serv->sv_lock);
>> ret = svc_xprt_local_port(newxprt);
>> + }
>> }
>> goto out;
>> }
>
> The nesting here is getting kind of deep.
>
> A bit more idiomatic for kernel code would be (untested) something like:
>
I agree. I'll look at refactoring this.
> list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
> struct svc_xprt *newxprt;
>
> if (strcmp(xprt_name, xcl->xcl_name) != 0)
> continue;
> spin_unlock(&svc_xprt_class_lock);
> if (!try_module_get(xcl->xcl_owner))
> goto out;
> # (assuming that's the right order)
> newxprt = xcl->xcl_ops->xpo_create (serv,
> (struct sockaddr *)&sin, sizeof(sin), flags);
> if (IS_ERR(newxprt)) {
> module_put(xcl->xcl_owner);
> ret = PTR_ERR(newxprt);
> goto out;
> }
> clear_bit(XPT_TEMP, &newxprt->xpt_flags);
> spin_lock_bh(&serv->sv_lock);
> list_add(&newxprt->xpt_list, &serv->sv_permsocks);
> spin_unlock_bh(&serv->sv_lock);
> ret = svc_xprt_local_port(newxprt);
> goto out;
> }
>
> At least to my eyes, that also makes it a lot easier to see what the
> important (succesful) case is, since that's the code that stays
> unindented all the way to the end.
>
> I'd also be inclined to replace those "got out"'s by "returns". I don't
> really see the point of the label when it's leads to something that's
> literally just a return.
>
> --b.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [EMAIL PROTECTED]
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
