[libvirt] [PATCH v3 8/9] rpc: pass listen FD to the daemon being started
Martin Kletzander
mkletzan at redhat.com
Thu Aug 14 09:23:27 UTC 2014
On Wed, Aug 13, 2014 at 04:09:09PM +0100, Daniel P. Berrange wrote:
>On Wed, Jul 23, 2014 at 04:27:12PM +0200, Martin Kletzander wrote:
>> This eliminates the need for active waiting.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>> src/rpc/virnetsocket.c | 102 ++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 83 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
>> index a94b2bc..46be541 100644
>> --- a/src/rpc/virnetsocket.c
>> +++ b/src/rpc/virnetsocket.c
>> @@ -1,7 +1,7 @@
>> /*
>> * virnetsocket.c: generic network socket handling
>> *
>> - * Copyright (C) 2006-2013 Red Hat, Inc.
>> + * Copyright (C) 2006-2014 Red Hat, Inc.
>> * Copyright (C) 2006 Daniel P. Berrange
>> *
>> * This library is free software; you can redistribute it and/or
>> @@ -121,7 +121,7 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket)
>>
>>
>> #ifndef WIN32
>> -static int virNetSocketForkDaemon(const char *binary)
>> +static int virNetSocketForkDaemon(const char *binary, int passfd)
>
>s/int/bool/ perhaps ?
>
Which one you mean? I'm keeping the return value as it was and the
passfd is the fd that will be passed.
[...]
>> @@ -569,28 +574,82 @@ int virNetSocketNewConnectUNIX(const char *path,
>> if (remoteAddr.data.un.sun_path[0] == '@')
>> remoteAddr.data.un.sun_path[0] = '\0';
>>
>> - retry:
>> - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
>> - if ((errno == ECONNREFUSED ||
>> - errno == ENOENT) &&
>> - spawnDaemon && retries < 20) {
>> - VIR_DEBUG("Connection refused for %s, trying to spawn %s",
>> - path, binary);
>> - if (retries == 0 &&
>> - virNetSocketForkDaemon(binary) < 0)
>> - goto error;
>> + if (spawnDaemon) {
>> + int err = 0;
>> + int rv = -1;
>> + int status = 0;
>> + pid_t pid = 0;
>>
>> - retries++;
>> - usleep(1000 * 100 * retries);
>> - goto retry;
>> + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
>> + virReportSystemError(errno, "%s", _("Failed to create socket"));
>> + goto error;
>> }
>>
>> - virReportSystemError(errno,
>> - _("Failed to connect socket to '%s'"),
>> + if (pipe2(errfd, O_CLOEXEC) < 0) {
>> + virReportSystemError(errno, "%s",
>> + _("Cannot create pipe for child"));
>> + 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.
>> + */
>> +
>> + if ((pid = virFork()) < 0)
>> + goto error;
>> +
>> + if (pid == 0) {
>> + VIR_FORCE_CLOSE(errfd[0]);
>> +
>> + umask(0077);
>> + rv = bind(passfd, &remoteAddr.data.sa, remoteAddr.len);
>> + if (rv < 0) {
>> + ignore_value(safewrite(errfd[1], &errno, sizeof(int)));
>> + }
>> + VIR_FORCE_CLOSE(errfd[1]);
>> + _exit(rv < 0 ? EXIT_FAILURE : EXIT_SUCCESS);
>> + }
>> +
>> + VIR_FORCE_CLOSE(errfd[1]);
>> + rv = virProcessWait(pid, &status, false);
>> + ignore_value(saferead(errfd[0], &err, sizeof(int)));
>> + VIR_FORCE_CLOSE(errfd[0]);
>> +
>> + if (rv < 0 || status != EXIT_SUCCESS) {
>> + if (err) {
>> + virReportSystemError(err,
>> + _("Failed to bind socket to %s "
>> + "in child process"),
>> + path);
>> + } else {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to bind socket to %s "
>> + "in child process"),
>> + path);
>> + }
>> + goto error;
>> + }
>> +
>> + if (listen(passfd, 0) < 0) {
>> + virReportSystemError(errno, "%s",
>> + _("Failed to listen on socket that's about "
>> + "to be passed to the daemon"));
>> + goto error;
>> + }
>
>Perhaps I'm miss-reading the code, but isn't this block gonig to
>result in failure if libvirtd is already running (& listening on
>the socket we try to bind to) and we requested auto-spawn ?
>
>> + }
>> +
>> + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
>> + virReportSystemError(errno, _("Failed to connect socket to '%s'"),
>> path);
>> goto error;
>> }
>
>Also I think there's still a race here because the already running
>libvirtd daemon could be unfortunately shutting down right at the
>same time we try to connect. So I think we still need to have a
>re-try loop around the connect attempt to address that race. Obviously
>it should be pretty damn rare now so won't have the problems that the
>current loop has.
>
Wouldn't connect(), bind(), connect() be enough (for both issues)? Or
do we need to try the dance few times again?
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140814/d60c73ee/attachment-0001.sig>
More information about the libvir-list
mailing list