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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list