[libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent

[Cole Robinson]

On 09/08/2014 01:46 AM, Martin Kletzander wrote:
> This way it behaves more like the daemon itself does (acquiring a
> pidfile, deleting the socket before binding, etc.).
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 42184bd..18a5a8f 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -51,9 +51,11 @@
>  #include "virlog.h"
>  #include "virfile.h"
>  #include "virthread.h"
> +#include "virpidfile.h"
>  #include "virprobe.h"
>  #include "virprocess.h"
>  #include "virstring.h"
> +#include "dirname.h"
>  #include "passfd.h"
> 
>  #if WITH_SSH2
> @@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path,
>                                 const char *binary,
>                                 virNetSocketPtr *retsock)
>  {
> +    char *binname = NULL;
> +    char *pidpath = NULL;
>      int fd, passfd = -1;
> +    int pidfd = -1;
>      virSocketAddr localAddr;
>      virSocketAddr remoteAddr;
> 
> @@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path,
>              goto error;
>          }
> 
> +        if (!(binname = last_component(binary)) || binname[0] == '\0') {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Cannot determine basename for binary '%s'"),
> +                           binary);
> +            goto error;
> +        }
> +
> +        if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0)
> +            goto error;
> +
> +        if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) {
> +            /*
> +             * This can happen in a very rare case of two clients spawning two
> +             * daemons at the same time, and the error in the logs that gets
> +             * reset here can be a clue to some future debugging.
> +             */
> +            virResetLastError();
> +            spawnDaemon = false;
> +            goto retry;
> +        }
> +
>          if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
>              virReportSystemError(errno, "%s", _("Failed to create socket"));
>              goto error;
>          }
> 
>          /*
> -         * We have to fork() here, because umask() is set
> -         * per-process, chmod() is racy and fchmod() has undefined
> -         * behaviour on sockets according to POSIX, so it doesn't
> -         * work outside Linux.
> +         * We already even acquired the pidfile, so noone else should be using
> +         * @path right now.  So we're OK to unlink it and paying attention to
> +         * the return value makes no real sense here.  Only if it's not an
> +         * abstract socket, of course.
> +         */
> +        if (path[0] != '@')
> +            unlink(path);
> +
> +        /*
> +         * We have to fork() here, because umask() is set per-process, chmod()
> +         * is racy and fchmod() has undefined behaviour on sockets according to
> +         * POSIX, so it doesn't work outside Linux.
>           */
>          if ((pid = virFork()) < 0)
>              goto error;
> @@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path,
>              /*
>               * OK, so the subprocces failed to bind() the socket.  This may mean
>               * that another daemon was starting at the same time and succeeded
> -             * with its bind().  So we'll try connecting again, but this time
> -             * without spawning the daemon.
> +             * with its bind() (even though it should not happen because we
> +             * using a pidfile for the race).  So we'll try connecting again,
> +             * but this time without spawning the daemon.
>               */
>              spawnDaemon = false;
>              goto retry;
> @@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path,
>              goto error;
>          }
> 
> +        /*
> +         * Do we need to eliminate the super-rare race here any more?  It would
> +         * need incorporating the following VIR_FORCE_CLOSE() into a
> +         * virCommandHook inside a virNetSocketForkDaemon().
> +         */
> +        VIR_FORCE_CLOSE(pidfd);
>          if (virNetSocketForkDaemon(binary, passfd) < 0)
>              goto error;
>      }
> @@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path,
>      return 0;
> 
>   error:
> +    if (pidfd > 0)
> +        virPidFileDeletePath(pidpath);
> +    VIR_FREE(pidpath);
>      VIR_FORCE_CLOSE(fd);
>      VIR_FORCE_CLOSE(passfd);
> +    VIR_FORCE_CLOSE(pidfd);
>      if (spawnDaemon)
>          unlink(path);
>      return -1;
> 

Tested this, fixes the issue for me as well.

Patch looks fine AFAICT, matches similar code stanzas in daemon/libvirtd.c. So
ACK from me

- Cole