4:23 p.m.
On 03/15/2011 11:51 AM, Daniel P. Berrange wrote:
Introduces a simple wrapper around the raw POSIX sockets APIs
and name resolution APIs. Allows for easy creation of client
and server sockets with correct usage of name resolution APIs
for protocol agnostic socket setup.
It can listen for UNIX and TCP stream sockets.
It can connect to UNIX, TCP streams directly, or indirectly
to UNIX sockets via an SSH tunnel or external command
* src/Makefile.am: Add to libvirt-net-rpc.la
* src/rpc/virnetsocket.c, src/rpc/virnetsocket.h: Generic
sockets APIs
---
.x-sc_avoid_write | 1 +
configure.ac | 2 +-
po/POTFILES.in | 1 +
src/Makefile.am | 3 +-
src/rpc/virnetsocket.c | 813 ++++++++++++++++++++++++++++++++++++++++++++++++
src/rpc/virnetsocket.h | 107 +++++++
6 files changed, 925 insertions(+), 2 deletions(-)
create mode 100644 src/rpc/virnetsocket.c
create mode 100644 src/rpc/virnetsocket.h
Looks like most (all?) of my earlier review comments were incorporated -
no more nasty double-close bugs :)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
new file mode 100644
index 0000000..a0eb431
--- /dev/null
+++ b/src/rpc/virnetsocket.c
@@ -0,0 +1,813 @@
+/*
+ * virnetsocket.h: generic network socket handling
+ *
+ * Copyright (C) 2006-2010 Red Hat, Inc.
Add 2011.
+#ifndef WIN32
+static int virNetSocketForkDaemon(const char *binary)
+{
+ int ret;
+ virCommandPtr cmd = virCommandNewArgList(binary,
+ "--timeout=30",
+ NULL);
+
+ virCommandAddEnvPassCommon(cmd);
+ virCommandClearCaps(cmd);
+ virCommandDaemonize(cmd);
+ ret = virCommandRun(cmd, NULL);
+ virCommandFree(cmd);
+ return ret;
+}
+#endif
Does this need an #else stub for mingw compilation, or is it only ever
called from code already excluded on mingw?
+
+
+static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr,
+ virSocketAddrPtr remoteAddr,
+ bool isClient,
+ int fd, int errfd, pid_t pid)
+{
+ virNetSocketPtr sock;
+ int no_slow_start = 1;
+
+ VIR_DEBUG("localAddr=%p remoteAddr=%p fd=%d errfd=%d pid=%d",
+ localAddr, remoteAddr,
+ fd, errfd, pid);
+
+ if (virSetCloseExec(fd) < 0 ||
+ virSetNonBlock(fd) < 0)
+ return NULL;
No error message? The virSet* functions are intentionally silent on
error, but this helper function should probably always issue an error on
all failure paths....
+
+ if (VIR_ALLOC(sock) < 0) {
+ virReportOOMError();
+ return NULL;
given that it already did so on this path, and that the caller can't
tell by a NULL return which error happened.
+ }
+
+ if (localAddr)
+ sock->localAddr = *localAddr;
+ if (remoteAddr)
+ sock->remoteAddr = *remoteAddr;
+ sock->fd = fd;
+ sock->errfd = errfd;
+ sock->pid = pid;
+
+ /* Disable nagle for TCP sockets */
+ if (sock->localAddr.data.sa.sa_family == AF_INET ||
+ sock->localAddr.data.sa.sa_family == AF_INET6)
+ setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
+ &no_slow_start,
+ sizeof(no_slow_start));
We don't care if setsockopt failed?
+
+int virNetSocketNewListenTCP(const char *nodename,
+ const char *service,
+ virNetSocketPtr **retsocks,
+ size_t *nretsocks)
+{
+ virNetSocketPtr *socks = NULL;
+ size_t nsocks = 0;
+ struct addrinfo *ai = NULL;
+ struct addrinfo hints;
+ int fd = -1;
+ int i;
+
+ *retsocks = NULL;
+ *nretsocks = 0;
+
+ memset (&hints, 0, sizeof hints);
My earlier review comment about ' (' vs. '(' consistency in function
calls still hasn't been addressed:
http://www.redhat.com/archives/libvir-list/2010-December/msg00675.html
+ int opt = 1;
+ setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof opt);
Again, no checks for setsockopt failures?
+error:
+ freeaddrinfo(ai);
+ for (i = 0 ; i < nsocks ; i++)
+ virNetSocketFree(socks[i]);
+ VIR_FREE(socks);
+ freeaddrinfo(ai);
Ouch - double freeaddrinfo - bound to segfault.
+
+ oldmask = umask(~mask);
+
+ if (bind(fd, &addr.data.sa, addr.len) < 0) {
+ virReportSystemError(errno,
+ _("Failed to bind socket to '%s'"),
+ path);
+ goto error;
+ }
+ umask(oldmask);
It's a shame that umask() is process-wide. This introduces a race
window to other threads. Is this a case where we need another
virFileOpenAs helper method, which forks, does the umask and bind in the
child, then passes the fd back to the parent? But that's a question for
another day, and doesn't affect the validity of this patch.
+
+
+#ifndef WIN32
+int virNetSocketNewConnectCommand(virCommandPtr cmd,
+ virNetSocketPtr *retsock)
+{
+ pid_t pid = 0;
+ int sv[2];
+ int errfd[2];
+
+ *retsock = NULL;
+
+ /* Fork off the external process. Use socketpair to create a private
+ * (unnamed) Unix domain socket to the child process so we don't have
+ * to faff around with two file descriptors (a la 'pipe(2)').
+ */
+ if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) {
+ virReportSystemError(errno, "%s",
+ _("unable to create socket pair"));
+ goto error;
+ }
+
+ if (pipe(errfd) < 0) {
Should we set the parent's half of sv and errfd to cloexec?
+error:
+ VIR_FORCE_CLOSE(sv[0]);
+ VIR_FORCE_CLOSE(sv[1]);
+ VIR_FORCE_CLOSE(errfd[0]);
+ VIR_FORCE_CLOSE(errfd[1]);
+
+ if (pid > 0) {
+ kill(pid, SIGTERM);
+ if (virCommandWait(cmd, NULL) < 0) {
+ kill(pid, SIGKILL);
+ if (virCommandWait(cmd, NULL) < 0) {
+ VIR_WARN("Unable to wait for command %d", pid);
Hmm, I really ought to write virCommandKill to make this idiom easier
(my virFileOpenAs patch can also use it).
+int virNetSocketAccept(virNetSocketPtr sock, virNetSocketPtr
*clientsock)
+{
+ int fd;
+ virSocketAddr localAddr;
+ virSocketAddr remoteAddr;
+
+ *clientsock = NULL;
+
+ memset(&localAddr, 0, sizeof(localAddr));
+ memset(&remoteAddr, 0, sizeof(remoteAddr));
+
+ remoteAddr.len = sizeof(remoteAddr.data.stor);
+ if ((fd = accept(sock->fd, &remoteAddr.data.sa, &remoteAddr.len)) < 0)
{
+ if (errno == ECONNABORTED ||
+ errno == EAGAIN)
+ return 0;
As written, this function returns 0 for both retry and success, and -1
for all other failure; it is up to the caller to check whether
*clientsock is NULL to know if a retry is needed. Should it return 0
for success and 1 for retry, to make it easier to use?
diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
new file mode 100644
index 0000000..c33b2e1
--- /dev/null
+++ b/src/rpc/virnetsocket.h
@@ -0,0 +1,107 @@
+/*
+ * virnetsocket.h: generic network socket handling
+ *
+ * Copyright (C) 2006-2010 Red Hat, Inc.
2011
No change to src/libvirt_private.syms to list all these new functions?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org