From: "Labiaga, Ricardo" Subject: RE: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs Date: Mon, 15 Jun 2009 08:31:02 -0700 Message-ID: <273FE88A07F5D445824060902F70034406334CCD@SACMVEXC1-PRD.hq.netapp.com> References: <4A35097B.70605@panasas.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: "Myklebust, Trond" , , To: "Benny Halevy" Return-path: Received: from mx2.netapp.com ([216.240.18.37]:23586 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751221AbZFOPbD convert rfc822-to-8bit (ORCPT ); Mon, 15 Jun 2009 11:31:03 -0400 In-Reply-To: <4A35097B.70605@panasas.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: > -----Original Message----- > From: Benny Halevy [mailto:bhalevy@panasas.com] > Sent: Sunday, June 14, 2009 7:30 AM > To: Labiaga, Ricardo > Cc: Myklebust, Trond; pnfs@linux-nfs.org; linux-nfs@vger.kernel.org > Subject: Re: [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs > > On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga > wrote: > > [squash with: nfs41: Implement NFSv4.1 callback service process] > > > > Signed-off-by: Ricardo Labiaga > > --- > > 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. > There are two lines in common between the #ifdef and #else case. + *rqstpp = nfs4_callback_up(serv); + *callback_svc = nfs4_callback_svc; Calling it code duplication is a bit of a stretch :-) - ricardo > 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 */ > *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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html