Re: [pnfs] [PATCH 08/14] SQUASHME: Removal of ugly #ifdefs

[Boaz Harrosh <bharrosh@panasas.com>]

On 06/14/2009 07:53 PM, Trond Myklebust wrote:
> On Sun, 2009-06-14 at 10:30 -0400, Benny Halevy wrote:
>> On Jun. 12, 2009, 1:54 -0400, Ricardo Labiaga <Ricardo.Labiaga@netapp.com> wrote:
>>> [squash with: nfs41: Implement NFSv4.1 callback service process]
>>>
>>> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
>>> ---
>>>  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....
> 
if you do somewhere global
#ifdef CONFIG_NFS_V4_1
	...
#else
	const int minorversion = 0;
#endif

Then the above if (minorversion) will be optimized out if not CONFIG_NFS_V4_1,
by the compiler. (And as a bonus it still gets parsed even if not defined)

>> 	*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
> 

Boaz