Re: [PATCH 4/6] NLM: Clean up nsm_monitor() call

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Dec 01, 2008 at 01:57:58PM -0500, Chuck Lever wrote:
> Clean up: A few minor clean-ups for nsm_monitor():

All looks fine, thanks, however:

> 
>  o  Make sure to return an error if the SM_MON call result is not zero.

I'd prefer to have a change in behavior split out into a separate patch
from pure cleanup.

--b.

> 
>  o  Remove the BUG_ON() -- the code will die anyway if nsm is NULL.
> 
>  o  Use nsm->sm_name instead of host->h_name to be consistent with
>     other functions in fs/lockd/mon.c.
> 
>  o  Collect the public declaration of nsm_monitor() in lockd.h with
>     other NSM public function declarations (eg. nsm_release).
> 
>  o  Add documenting comments.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c                 |   29 ++++++++++++++++++-----------
>  include/linux/lockd/lockd.h    |    4 ++++
>  include/linux/lockd/sm_inter.h |    1 -
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index a606fbb..78d5f59 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -69,18 +69,24 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
>  	return status;
>  }
>  
> -/*
> - * Set up monitoring of a remote host
> +/**
> + * nsm_monitor - Notify a peer in case we reboot
> + * @host: pointer to nlm_host of peer to notify
> + *
> + * If this peer is not already monitored, this function sends an
> + * upcall to the local rpc.statd to record the name/address of
> + * the peer to notify in case we reboot.
> + *
> + * Returns zero if the peer is monitored by the local rpc.statd;
> + * otherwise a negative errno value is returned.
>   */
> -int
> -nsm_monitor(struct nlm_host *host)
> +int nsm_monitor(const struct nlm_host *host)
>  {
>  	struct nsm_handle *nsm = host->h_nsmhandle;
> -	struct nsm_res	res;
> -	int		status;
> +	struct nsm_res res;
> +	int status;
>  
> -	dprintk("lockd: nsm_monitor(%s)\n", host->h_name);
> -	BUG_ON(nsm == NULL);
> +	dprintk("lockd: nsm_monitor(%s)\n", nsm->sm_name);
>  
>  	if (nsm->sm_monitored)
>  		return 0;
> @@ -92,9 +98,10 @@ nsm_monitor(struct nlm_host *host)
>  	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
>  
>  	status = nsm_mon_unmon(nsm, SM_MON, &res);
> -
> -	if (status < 0 || res.status != 0)
> -		printk(KERN_NOTICE "lockd: cannot monitor %s\n", host->h_name);
> +	if (res.status != 0)
> +		status = -EIO;
> +	if (status < 0)
> +		printk(KERN_NOTICE "lockd: cannot monitor %s\n", nsm->sm_name);
>  	else
>  		nsm->sm_monitored = 1;
>  	return status;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index de9ea7b..4ca6f39 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -233,6 +233,10 @@ extern void	  nlm_host_rebooted(const struct sockaddr_in *, const char *,
>  					unsigned int, u32);
>  void		  nsm_release(struct nsm_handle *);
>  
> +/*
> + * Host monitoring
> + */
> +int		  nsm_monitor(const struct nlm_host *host);
>  
>  /*
>   * This is used in garbage collection and resource reclaim
> diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
> index 5a5448b..546b610 100644
> --- a/include/linux/lockd/sm_inter.h
> +++ b/include/linux/lockd/sm_inter.h
> @@ -41,7 +41,6 @@ struct nsm_res {
>  	u32		state;
>  };
>  
> -int		nsm_monitor(struct nlm_host *);
>  int		nsm_unmonitor(struct nlm_host *);
>  extern int	nsm_local_state;
>  
>