[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 02/10] Introduce a generic object for using network sockets



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]