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

[Martin Kletzander]

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 at 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
-------------- 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/7de53137/attachment-0001.sig>