On Fri, Dec 12, 2008 at 04:57:42PM -0500, Chuck Lever wrote: > Clean up: I'd like to refactor __write_ports() to make it easier to > understand and maintain. Introduce a set of helper functions to > handle the details of the __write_ports() function. > > New helpers are not used yet. As stated in http://marc.info/?l=linux-nfs&m=122894134032274&w=2, I'd prefer that new code be introduced with its callers where reasonable, so in this case I'd rather this patch be combined with the following. One question: > +/* > + * A single 'fd' number was written, in which case it must be for > + * a socket of a supported family/protocol, and we use it as an > + * nfsd listener. > + */ > +static ssize_t __write_ports_addfd(char *buf, size_t size) > +{ > + char *mesg = buf; > + int fd, err; > + > + err = get_int(&mesg, &fd); > + if (err || fd < 0) > + return -EINVAL; > + > + err = nfsd_create_serv(); > + if (err) > + return err; > + > + err = svc_addsock(nfsd_serv, fd, buf); > + if (err < 0) > + return err; > + > + err = lockd_up(); > + if (err < 0) > + svc_sock_names(buf + strlen(buf) + 1, nfsd_serv, buf); > + > + /* Decrease the count, but don't shut down the the service */ > + nfsd_serv->sv_nrthreads--; Behavior on the error path seems slightly different here than in the original code, which did the sv_nrthreads-- even when svc_addsock failed. Could you check which is right? If the existing code is wrong, could you break out the fix into a separate patch? > + > + return err < 0 ? err : 0; > +} > + > +/* > + * A '-' followed by the 'name' of a socket means we close the socket. > + */ > +static ssize_t __write_ports_delfd(char *buf, size_t size) > +{ > + char *toclose; > + int len = 0; > + > + toclose = kstrdup(buf + 1, GFP_KERNEL); > + if (!toclose) > + return -ENOMEM; > + > + if (nfsd_serv) > + len = svc_sock_names(buf, nfsd_serv, toclose); > + if (len >= 0) > + lockd_down(); > + > + kfree(toclose); > + return len; > +} > + > +/* > + * A transport listener is added by writing it's transport name and > + * a port number > + */ > +static ssize_t __write_ports_addxprt(char *buf, size_t size, > + char *transport, unsigned short port) > +{ > + int err; > + > + err = nfsd_create_serv(); > + if (err) > + return err; > + > + err = svc_create_xprt(nfsd_serv, transport, port, SVC_SOCK_ANONYMOUS); > + if (err == -ENOENT) > + /* Give a reasonable perror msg for > + * bad transport string */ > + err = -EPROTONOSUPPORT; > + > + return err < 0 ? err : 0; > +} > + > +/* > + * A transport listener is removed by writing a "-", it's transport > + * name, and it's port number > + */ > +static ssize_t __write_ports_delxprt(char *buf, size_t size, > + char *transport, int port) > +{ > + struct svc_xprt *xprt; > + int err = -EINVAL; > + > + if (port == 0 || nfsd_serv == NULL) > + return err; > + > + xprt = svc_find_xprt(nfsd_serv, transport, AF_UNSPEC, port); > + if (xprt) { > + svc_close_xprt(xprt); > + svc_xprt_put(xprt); > + err = 0; > + } else > + err = -ENOTCONN; > + > + return err < 0 ? err : 0; I understand it's inherited from the original code, but the error-handling logic seems a bit silly; e.g., the "err" variable isn't really used. Why not: if (port == 0 || nfsd_serv == NULL) return -EINVAL; xprt = svc_find_xprt(...) if (!xprt) return -ENOTCONN; svc_close_xprt(xprt); svc_xprt_put(xprt); return 0; ? This patch and the following look good otherwise. --b. > +} > + > static ssize_t __write_ports(struct file *file, char *buf, size_t size) > { > if (size == 0) { > -- 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
