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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list