On Thu, 4 Jun 2009 16:00:43 -0400
Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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.
>
Hmm good catch -- I'll admit that I never bothered to look at what BUFSIZ
actually is. SIMPLE_TRANSACTION_LIMIT sounds more reasonable, but we
can probably get away with even less than that for this.
> > - 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);" ?
>
Good catch -- will fix.
> > + 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().
>
I think that gets removed later (part of the problem with trying to
break up huge patches into smaller ones is that you miss stuff like
this). I'll see if it can be removed here.
> >
> > + 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
Thanks!
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
