On Dec 24, 2008, at Dec 24, 2008, 11:15 PM, J. Bruce Fields wrote:
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.
Right, I wrote these patches well before the referenced e-mail, and
split them this way because the diff of the combined changes is well-
nigh impossible to read.
What I may end up doing instead is splitting these helpers out one
patch at a time. That should make for more legible patches.
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) {
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
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
