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:
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.
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
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:
>
> 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.
>
Agree. Fixed.
> --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