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

Re: [libvirt] [PATCH v3 4/9] daemon: support passing FDs from the calling process



On Wed, Aug 13, 2014 at 04:01:42PM +0100, Daniel P. Berrange wrote:
On Wed, Jul 23, 2014 at 04:27:08PM +0200, Martin Kletzander wrote:
First FD is the RW unix socket to listen on, second one (if
applicable) is the RO unix socket.

Signed-off-by: Martin Kletzander <mkletzan redhat com>
---
 daemon/libvirtd.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4c926b3..15f0ce2 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -56,6 +56,7 @@
 #include "virstring.h"
 #include "locking/lock_manager.h"
 #include "viraccessmanager.h"
+#include "virutil.h"

 #ifdef WITH_DRIVER_MODULES
 # include "driver.h"
@@ -476,11 +477,19 @@ static int daemonSetupNetworking(virNetServerPtr srv,
     int unix_sock_ro_mask = 0;
     int unix_sock_rw_mask = 0;

+    unsigned int cur_fd = STDERR_FILENO + 1;
+    unsigned int nfds = virGetListenFDs();
+
     if (config->unix_sock_group) {
         if (virGetGroupID(config->unix_sock_group, &unix_sock_gid) < 0)
             return -1;
     }

+    if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) {
+        VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds);
+        return -1;
+    }
+
     if (virStrToLong_i(config->unix_sock_ro_perms, NULL, 8, &unix_sock_ro_mask) != 0) {
         VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_ro_perms);
         goto error;
@@ -491,30 +500,30 @@ static int daemonSetupNetworking(virNetServerPtr srv,
         goto error;
     }

-    VIR_DEBUG("Registering unix socket %s", sock_path);
-    if (!(svc = virNetServerServiceNewUNIX(sock_path,
-                                           unix_sock_rw_mask,
-                                           unix_sock_gid,
-                                           config->auth_unix_rw,
+    if (!(svc = virNetServerServiceNewFDOrUNIX(sock_path,
+                                               unix_sock_rw_mask,
+                                               unix_sock_gid,
+                                               config->auth_unix_rw,
 #if WITH_GNUTLS
-                                           NULL,
+                                               NULL,
 #endif
-                                           false,
-                                           config->max_queued_clients,
-                                           config->max_client_requests)))
+                                               false,
+                                               config->max_queued_clients,
+                                               config->max_client_requests,
+                                               nfds, &cur_fd)))
         goto error;
     if (sock_path_ro) {
-        VIR_DEBUG("Registering unix socket %s", sock_path_ro);
-        if (!(svcRO = virNetServerServiceNewUNIX(sock_path_ro,
-                                                 unix_sock_ro_mask,
-                                                 unix_sock_gid,
-                                                 config->auth_unix_ro,
+        if (!(svcRO = virNetServerServiceNewFDOrUNIX(sock_path_ro,
+                                                     unix_sock_ro_mask,
+                                                     unix_sock_gid,
+                                                     config->auth_unix_ro,
 #if WITH_GNUTLS
-                                                 NULL,
+                                                     NULL,
 #endif
-                                                 true,
-                                                 config->max_queued_clients,
-                                                 config->max_client_requests)))
+                                                     true,
+                                                     config->max_queued_clients,
+                                                     config->max_client_requests,
+                                                     nfds, &cur_fd)))
             goto error;
     }

I'm wondering how we deal with TCP socket listening too. eg if someone
modifies the systemd config so that it listens on the TCP sockets too
then we're not setting up libvirt to deal with those.


Is this really necessary?  I mean I went with the approach that we can
get more than 1 FD passed even though it's not needed to fix the 20s
timeout (the original bug).  And if we decide we need more it can be
reworked that way, yes.  But right now I really don't think we do,
especially since race condition solved by passing TCP or TLS ports
would be *really* rare; and anyone connecting during the time system
daemon is starting will just try again in a while.

Rather than doing this NewFDOrUnix() approach, I'm wondering if we
would be better off doing.

  if (getenv(LISTEN_FDS) != NULL) {
     ....call NewFD() on all the FDs we've been given...
  } else {
     .. do normal NewUnix/NewTCP/NewTLS setup...
  }

though it gets harder than that because we'll need to call getsockname
on the FD to determine if the socket we've received is a UNIX or TCP
socket and what port number it is on (so we can see if it is TCP or
TLS protocol) and we need to passin the TLS context in some cases.

So perhaps the  virNetServerServiceNewTCP and virNetServerServiceNewTLS
method should simply accept a pre-opened FD. so we could do


  int unixfd = virGetInheritedUNIXSocket("/some/path");
  int unixrofd = virGetInheritedTCPSocket("/some/other/path")
  int tcpfd = virGetInheritedTCPSocket(16509)
  int tlsfd = virGetInheritedTLSSocket(16514)

and then pass these FDs into the regular virNetServerServiceNew*
methods. Assume '-1' means pre-open the socket the normal way


I don't think right now this approach would add as much benefit as how
much code it would add (and with this potential problems, too).  I
think that from the .service file it is pretty obvious that we just
support passing FDs for the sockets.

Martin

Attachment: signature.asc
Description: Digital signature


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