On Sun, 2009-06-14 at 10:30 -0400, Benny Halevy wrote:
> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@xxxxxxxxxx> wrote:
> > [squash with: nfs41: Implement NFSv4.1 callback service process]
> >
> > Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@xxxxxxxxxx>
> > ---
> > fs/nfs/callback.c | 50 +++++++++++++++++++++++++++++++++++---------------
> > 1 files changed, 35 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > index 4395c96..116424e 100644
> > --- a/fs/nfs/callback.c
> > +++ b/fs/nfs/callback.c
> > @@ -209,6 +209,39 @@ out:
> > dprintk("--> %s return %p\n", __func__, rqstp);
> > return rqstp;
> > }
> > +
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > + struct svc_serv *serv, struct rpc_xprt *xprt,
> > + struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> > +{
> > + if (minorversion) {
> > + *rqstpp = nfs41_callback_up(serv, xprt);
> > + *callback_svc = nfs41_callback_svc;
> > + } else {
> > + *rqstpp = nfs4_callback_up(serv);
> > + *callback_svc = nfs4_callback_svc;
> > + }
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> > + struct nfs_callback_data *cb_info)
> > +{
> > + if (minorversion)
> > + xprt->bc_serv = cb_info->serv;
> > +}
> > +#else
> > +static inline void nfs_callback_svc_setup(u32 minorversion,
> > + struct svc_serv *serv, struct rpc_xprt *xprt,
> > + struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> > +{
> > + *rqstpp = nfs4_callback_up(serv);
> > + *callback_svc = nfs4_callback_svc;
> > +}
> > +
> > +static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> > + struct nfs_callback_data *cb_info)
> > +{
> > +}
> > #endif /* CONFIG_NFS_V4_1 */
>
> Although this removes the ungly #ifdefs from inside nfs_callback_up
> it just introduces them elsewhere and what's worse, it adds code
> duplication which is even more unreadable and harder to maintain.
>
> How about something along these line?
>
> static inline void nfs_callback_svc_setup(u32 minorversion,
> struct svc_serv *serv, struct rpc_xprt *xprt,
> struct svc_rqst **rqstpp, int (**callback_svc)(void *vrqstp))
> {
> #ifdef CONFIG_NFS_V4_1
> if (minorversion) {
> *rqstpp = nfs41_callback_up(serv, xprt);
> *callback_svc = nfs41_callback_svc;
> return;
> }
> #endif /* CONFIG_NFS_V4_1 */\
NACK.... That's _exactly_ the kind of unreadable nonsense I _DON'T_ want
to see....
> *rqstpp = nfs4_callback_up(serv);
> *callback_svc = nfs4_callback_svc;
> }
>
> static inline void nfs_callback_bc_serv(u32 minorversion, struct rpc_xprt *xprt,
> struct nfs_callback_data *cb_info)
> {
> #ifdef CONFIG_NFS_V4_1
> if (minorversion)
> xprt->bc_serv = cb_info->serv;
> #endif /* CONFIG_NFS_V4_1 */
> }
>
> Benny
>
> >
> > /*
> > @@ -225,10 +258,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
> >
> > mutex_lock(&nfs_callback_mutex);
> > if (cb_info->users++ || cb_info->task != NULL) {
> > -#if defined(CONFIG_NFS_V4_1)
> > - if (minorversion)
> > - xprt->bc_serv = cb_info->serv;
> > -#endif /* CONFIG_NFS_V4_1 */
> > + nfs_callback_bc_serv(minorversion, xprt, cb_info);
> > goto out;
> > }
> > serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
> > @@ -239,17 +269,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
> >
> > /* FIXME: either 4.0 or 4.1 callback service can be up at a time
> > * need to monitor and control them both */
> > - if (!minorversion) {
> > - rqstp = nfs4_callback_up(serv);
> > - callback_svc = nfs4_callback_svc;
> > - } else {
> > -#if defined(CONFIG_NFS_V4_1)
> > - rqstp = nfs41_callback_up(serv, xprt);
> > - callback_svc = nfs41_callback_svc;
> > -#else /* CONFIG_NFS_V4_1 */
> > - BUG();
> > -#endif /* CONFIG_NFS_V4_1 */
> > - }
> > + nfs_callback_svc_setup(minorversion, serv, xprt, &rqstp, &callback_svc);
> >
> > if (IS_ERR(rqstp)) {
> > ret = PTR_ERR(rqstp);
>
> --
> 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
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
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
