1:45 p.m.
On Tue, Mar 15, 2011 at 03:23:38PM -0600, Eric Blake wrote:
On 03/15/2011 11:51 AM, Daniel P. Berrange wrote:
> +#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?
No, we simply never call it, because the UNIX socket code
is also defined out.
> +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....
Yep, good point.
> + }
> +
> + 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?
Yes, we should report errors
> +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
I got quite a few of them, but missed more.
> + 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.
Opps.
> + 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.
Fortunately socket bind takes place during main daemon startup
when there is only 1 thread. In general though, it would be nice
to have a way to address this.
> +
> +
> +#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?
Not sure its really worth it, given the rest of our
codebase.
> +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?
This does not actually mean 'retry'. These "errors" occur if
the client closed its TCP connection between the time of
poll() and accept(). So callers do something like this:
if (virNetSocketAccept(sock, &client) < 0)
return -1
if (!client)
return 0;
...work with new client
return 0;
> 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?
Nothing required this (yet)
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|