Clean up: The makesock() function can become static since it is only used in
rpcmisc.c, where it is defined. Fix some minor nits while we're in the area:
o Move it so we can remove it's forward declaration.
o Get rid of unneeded newlines in the xlog() format strings.
o Use htonl(INADDR_ANY) instead of INADDR_ANY to initialize sin_addr.
Should make no run-time difference, but is slightly more proper,
as the standard definition of INADDR_ANY is in host byte-order.
o Return RPC_ANYSOCK on error (this is -1) instead of open-coding -1.
o Remove the parentheses in the "return" statements.
Signed-off-by: Chuck Lever <[email protected]>
---
support/include/rpcmisc.h | 1 -
support/nfs/rpcmisc.c | 76 ++++++++++++++++++++++++---------------------
2 files changed, 40 insertions(+), 37 deletions(-)
diff --git a/support/include/rpcmisc.h b/support/include/rpcmisc.h
index 35c5011..5814a63 100644
--- a/support/include/rpcmisc.h
+++ b/support/include/rpcmisc.h
@@ -42,7 +42,6 @@ struct rpc_dtable {
}
-int makesock(int port, int proto);
void rpc_init(char *name, int prog, int vers,
void (*dispatch)(struct svc_req *, SVCXPRT *),
int defport);
diff --git a/support/nfs/rpcmisc.c b/support/nfs/rpcmisc.c
index ad53a43..49e88e8 100644
--- a/support/nfs/rpcmisc.c
+++ b/support/nfs/rpcmisc.c
@@ -38,8 +38,6 @@
#define socklen_t int
#endif
-int makesock(int port, int proto);
-
#define _RPCSVC_CLOSEDOWN 120
int _rpcpmstart = 0;
int _rpcfdtype = 0;
@@ -70,6 +68,46 @@ closedown(int sig)
(void) alarm(_RPCSVC_CLOSEDOWN);
}
+/*
+ * Create listener socket for a given port
+ *
+ * Return an open network socket on success; otherwise return
+ * RPC_ANYSOCK if some error occurs.
+ */
+static int
+makesock(int port, int proto)
+{
+ struct sockaddr_in sin;
+ int sock, sock_type, val;
+
+ sock_type = (proto == IPPROTO_UDP) ? SOCK_DGRAM : SOCK_STREAM;
+ sock = socket(AF_INET, sock_type, proto);
+ if (sock < 0) {
+ xlog(L_FATAL, "Could not make a socket: %s",
+ strerror(errno));
+ return RPC_ANYSOCK;
+ }
+ memset((char *) &sin, 0, sizeof(sin));
+ sin.sin_family = AF_INET;
+ sin.sin_addr.s_addr = htonl(INADDR_ANY);
+ sin.sin_port = htons(port);
+
+ val = 1;
+ if (proto == IPPROTO_TCP)
+ if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
+ &val, sizeof(val)) < 0)
+ xlog(L_ERROR, "setsockopt failed: %s",
+ strerror(errno));
+
+ if (bind(sock, (struct sockaddr *) &sin, sizeof(sin)) == -1) {
+ xlog(L_FATAL, "Could not bind name to socket: %s",
+ strerror(errno));
+ return RPC_ANYSOCK;
+ }
+
+ return sock;
+}
+
void
rpc_init(char *name, int prog, int vers,
void (*dispatch)(struct svc_req *, register SVCXPRT *),
@@ -167,37 +205,3 @@ rpc_init(char *name, int prog, int vers,
alarm (_RPCSVC_CLOSEDOWN);
}
}
-
-int makesock(int port, int proto)
-{
- struct sockaddr_in sin;
- int s;
- int sock_type;
- int val;
-
- sock_type = (proto == IPPROTO_UDP) ? SOCK_DGRAM : SOCK_STREAM;
- s = socket(AF_INET, sock_type, proto);
- if (s < 0) {
- xlog(L_FATAL, "Could not make a socket: %s\n",
- strerror(errno));
- return (-1);
- }
- memset((char *) &sin, 0, sizeof(sin));
- sin.sin_family = AF_INET;
- sin.sin_addr.s_addr = INADDR_ANY;
- sin.sin_port = htons(port);
-
- val = 1;
- if (proto == IPPROTO_TCP)
- if (setsockopt(s, SOL_SOCKET, SO_REUSEADDR,
- &val, sizeof(val)) < 0)
- xlog(L_ERROR, "setsockopt failed: %s\n",
- strerror(errno));
-
- if (bind(s, (struct sockaddr *) &sin, sizeof(sin)) == -1) {
- xlog(L_FATAL, "Could not bind name to socket: %s\n",
- strerror(errno));
- return (-1);
- }
- return (s);
-}
On Thursday September 4, [email protected] wrote:
>
> o Return RPC_ANYSOCK on error (this is -1) instead of open-coding -1.
I don't agree with this change.
Everything else (including the last) looks fine, though I personally
wouldn't bother closing file descriptors and freeing memory just
before exit.
RPC_ANYSOCK is an input value to various rpc library functions which
means "I haven't opened a socket, you do it for me". It is not really
a valid return value.
Every time makesock is called, the return status is tested with
if (makesock(...) < 0)
To me, that says that the returned value should be e.g. -1, not
RPC_ANYSOCK.
Thanks,
NeilBrown
On Thu, Sep 4, 2008 at 9:14 PM, Neil Brown <[email protected]> wrote:
> On Thursday September 4, [email protected] wrote:
>>
>> o Return RPC_ANYSOCK on error (this is -1) instead of open-coding -1.
>
> I don't agree with this change.
>
> Everything else (including the last) looks fine, though I personally
> wouldn't bother closing file descriptors and freeing memory just
> before exit.
If nothing else, it makes interpreting the output of valgrind easier. :-)
Code in nfs-utils seems to last many many years, and is copied and
pasted a lot without regard to context (ie code that may have been
written for a single-shot application may suddenly find itself
embedded in a long-running daemon). Considering that parts of
nfs-utils are used in HA environments, I'd rather be safe then sorry.
> RPC_ANYSOCK is an input value to various rpc library functions which
> means "I haven't opened a socket, you do it for me". It is not really
> a valid return value.
Well, FWIW it is treated as a return value by
utils/mount/network.c:get_socket() and it's callers.
> Every time makesock is called, the return status is tested with
> if (makesock(...) < 0)
>
> To me, that says that the returned value should be e.g. -1, not
> RPC_ANYSOCK.
OK, I'll drop it. Thanks for the review.
--
Chuck Lever