On 12/16/2010 04:21 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
> +++ b/.x-sc_avoid_write
> @@ -1,6 +1,7 @@
> ^src/libvirt\.c$
> ^src/fdstream\.c$
> ^src/qemu/qemu_monitor\.c$
> +^src/rpc/virnetsocket.c$
Technically, and for consistency, that should be \. rather than . in the
regex.
> +
> +static int virNetSocketForkDaemon(const char *binary)
> +{
> + const char *const daemonargs[] = { binary, "--timeout=30", NULL };
> + pid_t pid;
> +
> + if (virExecDaemonize(daemonargs, NULL, NULL,
> + &pid, -1, NULL, NULL,
> + VIR_EXEC_CLEAR_CAPS,
> + NULL, NULL, NULL) < 0)
virCommand?
>
> +static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr,
> + sock->localAddr = *localAddr;
This line requires that localAddr be non-NULL...
> + if (!(sock->localAddrStr = virSocketFormatAddrFull(localAddr, true, ";")))
> + goto error;
> +
> + if (remoteAddr &&
> + !(sock->remoteAddrStr = virSocketFormatAddrFull(remoteAddr, true, ";")))
> + goto error;
> +
> + sock->client = localAddr && !remoteAddr ? false : true;
therefore, this line could be simplified to:
sock->client = !!remoteAddr;
> +
> + VIR_DEBUG("sock=%p localAddrStr=%s remoteAddrStr=%s",
> + sock, NULLSTR(sock->localAddrStr), NULLSTR(sock->remoteAddrStr));
and here, you only need the NULLSTR() for remoteAddrStr.
> +
> + return sock;
> +
> +error:
> + virNetSocketFree(sock);
Double close() problem. Right now, the semantics appear to be that on
success, sock owns fd, and fd will be closed later during
virNetSocketFree(); but on failure, the caller is responsible for
closing fd (well, the majority of callers followed this rule; more on
this later).
But if this function fails during virSocketFormatAddrFull, then both
this error path and the caller will close fd. You need to explicitly
set sock->fd = sock->errfd = -1 prior to calling virNetSocketFree() on
this error path, or else change the semantics so that this function
takes ownership of fd and the caller must not close fd after calling
this function. (Of the two choices, I'd go with the first).
> +int virNetSocketNewListenTCP(const char *nodename,
> +
> + memset (&hints, 0, sizeof hints);
> + hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
> + hints.ai_socktype = SOCK_STREAM;
> +
> + int e = getaddrinfo (nodename, service, &hints, &ai);
Why the spaces before (? The entire file has several inconsistencies
between function() and function ().
> + struct addrinfo *runp = ai;
> + while (runp) {
> + virSocketAddr addr;
> +
> + memset(&addr, 0, sizeof(addr));
> +
> + if ((fd = socket(runp->ai_family, runp->ai_socktype,
> + runp->ai_protocol)) < 0) {
Still on my gnulib TODO list - support SOCK_CLOEXEC and SOCK_NONBLOCK
Linux extensions on all platforms (on newer Linux and cygwin, it gives
us the benefit of race-free cloexec and fewer syscalls; elsewhere the
cloexec race is still present but libvirt code can be simplified).
Doesn't affect the quality of this patch, though.
By the way, on today's POSIX teleconference, the issue was raised that
blindly close()ing all fd's after fork() is technically non-compliant,
since it risks accidentally closing out any implementation fd's such as
used in posix_trace_* functions (but seldom an issue in real life, since
posix_trace_* is seldom used); so the POSIX folks agreed that the POSIX
standard needs to add in dup3, pipe2, accept4, and SOCK_CLOEXEC to
parallel the POSIX 2008 addition of O_CLOEXEC and F_DUPFD_CLOEXEC, in
order to provide a reliable counterpart to avoid the cloexec race
without mistakenly closing too many fds in the child.
> + */
> + setsockopt(fd, IPPROTO_IPV6,IPV6_V6ONLY,
> + (void*)&on, sizeof on);
The cast to void* shouldn't be necessary.
> + if (!(socks[nsocks-1] = virNetSocketNew(&addr, NULL, fd, -1, 0)))
> + goto error;
> + runp = runp->ai_next;
> + }
> +
> + freeaddrinfo (ai);
> +
> + *retsocks = socks;
> + *nretsocks = nsocks;
> + return 0;
> +
> +error:
> + VIR_FORCE_CLOSE(fd);
> + return -1;
Resource leak. You need to iterate over socks with virNetSocketFree()
followed by VIR_FREE(socks). Also, conditionally call freeaddrinfo(ai),
depending on whether getaddrinfo succeeded.
>
> +static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr,
> + if (bind(fd, &addr.data.sa, addr.len) < 0) {
> + virReportSystemError(errno,
> + _("Failed to bind socket to '%s'"),
> + path);
> + goto error;
> + }
> + umask(oldmask);
> + if (grp != 0 && setgid(oldgrp)) {
> + virReportSystemError(errno,
> + _("Failed to restore group ID to %d"), oldgrp);
> + goto error;
> + }
> +
> + if (!(*retsock = virNetSocketNew(&addr, NULL, fd, -1, 0)))
> + goto error;
> +
> + return 0;
> +
> +error:
> + VIR_FORCE_CLOSE(fd);
> + return -1;
Should this error path attempt to unlink any file created by bind() but
discarded when setgid() failed, if path did not start with '@'?
> +int virNetSocketNewConnectTCP(const char *nodename,
> + runp = ai;
> + while (runp) {
> + if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0)
> + break;
> +
> + savedErrno = errno;
> + VIR_FORCE_CLOSE(fd);
Do we want to save the first failure, rather than the last failure?
> + if (fd == -1) {
> + virReportSystemError(savedErrno,
> + _("unable to connect to server at '%s:%s'"),
> + nodename, service);
> + return -1;
> + }
> +
> + freeaddrinfo (ai);
Resource leak. Move this line to occur before the fd == -1 check.
> +
> + if (VIR_EXPAND_N(socks, nsocks, 1) < 0) {
Huh? socks and nsocks have no use in this function.
> +error:
> + VIR_FORCE_CLOSE(fd);
> + return -1;
Again, conditionally call freeaddrinfo(ai).
> +int virNetSocketNewConnectCommand(virCommandPtr cmd,
> +
> + if (pid > 0 &&
> + virCommandWait(cmd, NULL) < 0)
> + VIR_WARN("Unable to wait for command %d", pid);
Should you attempt to kill(pid, SIGTERM) prior to virCommandWait() to
give the child process heads up that it must exit early? Should
virCommand be taught a new interface to make it easy to send SIGTERM,
and if that doesn't work within a given timeout, to follow it up with
SIGKILL, to make it easy to abort a child due to external reasons like this?
> +int virNetSocketNewConnectSSH(const char *nodename,
> + if (noTTY)
> + virCommandAddArgList(cmd, "-T", "-o", "BatchMode=yes",
> + "-e", "none", NULL);
> + virCommandAddArgList(cmd, nodename,
> + netcat ? netcat : "nc",
> + "-U", path, NULL);
> +
> + return virNetSocketNewConnectCommand(cmd, retsock);
This is just awesome how compact virCommand makes things :)
> +int virNetSocketNewConnectExternal(const char **cmdargv,
> + virNetSocketPtr *retsock)
> +{
> + virCommandPtr cmd;
> + int i;
s/int/size_t/, if you keep a loop, but...
> +
> + *retsock = NULL;
> +
> + cmd = virCommandNew(cmdargv[0]);
cmd = virCommandNewArgs(cmdargv);
(although you might have to add in a cast)...
> + virCommandAddEnvPassCommon(cmd);
> + virCommandClearCaps(cmd);
> +
> + for (i = 1 ; cmdargv[i] != NULL ; i++)
> + virCommandAddArg(cmd, cmdargv[i]);
...and then you don't need a loop.
> +int virNetSocketAccept(virNetSocketPtr sock, virNetSocketPtr *clientsock)
> + if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) {
> + virReportSystemError(errno, "%s", _("Unable to get local socket name"));
> + VIR_FORCE_CLOSE(fd);
> + return -1;
> + }
> +
> + if (!(*clientsock = virNetSocketNew(&localAddr,
> + &remoteAddr,
> + fd, -1, 0)))
> + return -1;
Resource leak - close fd on failure (this was the one I hinted at earlier).
> +void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
> +{
> + if (sock->watch <= 0) {
> + VIR_DEBUG("Watch not registered on socket %p", sock);
> + return;
> + }
> +
> + virEventRemoveHandle(sock->watch);
Shouldn't this be:
sock->watch = virEventRemoveHandle(sock->watch);
--
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