Re: [PATCH 05/10] nfs-utils: move check for active knfsd to helper function

[Chuck Lever <chuck.lever@oracle.com>]

On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:

> nfssvc_setfds checks to see if knfsd is already running. Move this
> check to a helper function. Eventually the nfsd code will call this
> directly.

Some of this isn't your fault, but this code could use a little extra  
clean up as you split it up, IMO.

> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> support/nfs/nfssvc.c |   44 ++++++++++++++++++++++++++++++ 
> +-------------
> 1 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c
> index 3e6bd31..8b15c4d 100644
> --- a/support/nfs/nfssvc.c
> +++ b/support/nfs/nfssvc.c
> @@ -24,28 +24,46 @@
> #define NFSD_VERS_FILE    "/proc/fs/nfsd/versions"
> #define NFSD_THREAD_FILE  "/proc/fs/nfsd/threads"
>
> -static void
> -nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> +/*
> + * Are there already sockets configured? If not, then it is safe to  
> try to
> + * open some and pass them through.
> + *
> + * Note: If the user explicitly asked for 'udp', then we should  
> probably check
> + * if that is open, and should open it if not. However we don't  
> yet. All
> + * sockets have to be opened when the first daemon is started.
> + */
> +int
> +nfssvc_inuse(void)
> {
> -	int fd, n, on=1;
> +	int fd, n;

read(2) returns a ssize_t result, not an int.

> 	char buf[BUFSIZ];

Eesh.  BUFSIZ looks like it's two pages on my system, so adding this  
helper makes the stack requirements in this path over 16KB.

Might be better if we used something a little more stack friendly  
here.  The kernel bounds the size of the read(2) result to  
SIMPLE_TRANSACTION_LIMIT, which is less than a page, for example.   
Making it a static would keep "buf" off the stack, too; or checking  
how much buffer we really need (like only a few bytes, for this  
particular check) might be better.

> -	int udpfd = -1, tcpfd = -1;
> -	struct sockaddr_in sin;
>
> 	fd = open(NFSD_PORTS_FILE, O_RDONLY);
> +
> +	/* problem opening file, assume that nothing is configured */
> 	if (fd < 0)
> -		return;
> +		return 0;
> +
> 	n = read(fd, buf, BUFSIZ);

Nit: Using "sizeof(buf)" might be slightly more maintainable than  
"BUFSIZ".

> 	close(fd);
> +
> 	if (n != 0)

If read(2) returns -1, we return 1 here.  How about "return (n > 0);" ?

> +		return 1;
> +
> +	return 0;
> +}
> +
> +static void
> +nfssvc_setfds(int port, unsigned int ctlbits, char *haddr)
> +{
> +	int fd, n, on=1;

Another compiler warning issue: "n" is probably now unused in  
nfssvc_setfds().

>
> +	char buf[BUFSIZ];
> +	int udpfd = -1, tcpfd = -1;
> +	struct sockaddr_in sin;
> +
> +	if (nfssvc_inuse())
> 		return;
> -	/* there are no ports currently open, so it is safe to
> -	 * try to open some and pass them through.
> -	 * Note: If the user explicitly asked for 'udp', then
> -	 * we should probably check if that is open, and should
> -	 * open it if not.  However we don't yet.  All sockets
> -	 * have to be opened when the first daemon is started.
> -	 */
> +
> 	fd = open(NFSD_PORTS_FILE, O_WRONLY);
> 	if (fd < 0)
> 		return;
> -- 
> 1.6.2.2
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com