chardev: unconditionally set FD_PASS feature for socket type=fd

The vhostuser network backend requires the chardev it is using to have the FD passing feature. It checks this upfront when initializing the network backend and reports an error if not set.

The socket chardev has to set the FD_PASS feature during early initialization to satisfy the vhostuser backend, and at this point the socket has not been initialized. It is thus unable to do a live check on the socket to see if it supports FD passing (aka is a UNIX socket). As a result it has to blindly set FD_PASS feature based solely on info in the SocketAddress struct, such as address type.

Unfortunately libvirt wishes to use FD passing to provide the UNIX domain socket listener, and as a result the FD_PASS feature is no longer set, which breaks vhostuser's checks, despite the fact that FD passing will in fact work later.

This unconditionally sets FD_PASS feature for any socket address which has type==fd. Thus will be wrong if the passed in FD was not a UNIX socket, but if an attempt is later made to use FD passing we'll still get an error reported by the QIOChannelSocket class. So the effective of setting the chardev FD_PASS feature early is merely to delay error reporting.

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 chardev/char-socket.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

chardev/char-socket.c +12 -2

index 17519ec589..b495d6a851 100644
@@ -990,8 +990,18 @@ static void qmp_chardev_open_socket(Chardev *chr,
     s->addr = addr = socket_address_flatten(sock->addr);
 
     qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
-    /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
-    if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
+    /*
+     * We can't tell at this point if the "fd" we're passed is
+     * a UNIX socket or not, so can't reliably set the
+     * FD_PASS feature. vhost-user, however, checks for this
+     * feature early before we've even created the I/O channel,
+     * so we can't wait until later to set the feature. Thus
+     * we optimistically set the FD_PASS feature. If the passed
+     * in "fd" is not a UNIX socket, there will be an error
+     * reported later anyway.
+     */
+    if (addr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+        addr->type == SOCKET_ADDRESS_TYPE_FD) {
         qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
     }
 

Quoted 16 lines · Daniel P. Berrangé

The vhostuser network backend requires the chardev it is using to have the FD passing feature. It checks this upfront when initializing the network backend and reports an error if not set.

The socket chardev has to set the FD_PASS feature during early initialization to satisfy the vhostuser backend, and at this point the socket has not been initialized. It is thus unable to do a live check on the socket to see if it supports FD passing (aka is a UNIX socket). As a result it has to blindly set FD_PASS feature based solely on info in the SocketAddress struct, such as address type.

Unfortunately libvirt wishes to use FD passing to provide the UNIX domain socket listener, and as a result the FD_PASS feature is no longer set, which breaks vhostuser's checks, despite the fact that FD passing will in fact work later.

Quoted 6 lines

This unconditionally sets FD_PASS feature for any socket address which has type==fd. Thus will be wrong if the passed in FD was not a UNIX socket, but if an attempt is later made to use FD passing we'll still get an error reported by the QIOChannelSocket class. So the effective of setting the chardev FD_PASS feature early is merely to delay error reporting.

Could you query with getsockopt or getsockname in tcp_chr_connect?

Quoted 24 lines · Paolo Bonzini

Quoted 22 lines · Daniel P. Berrangé

The vhostuser network backend requires the chardev it is using to have the FD passing feature. It checks this upfront when initializing the network backend and reports an error if not set.

The socket chardev has to set the FD_PASS feature during early initialization to satisfy the vhostuser backend, and at this point the socket has not been initialized. It is thus unable to do a live check on the socket to see if it supports FD passing (aka is a UNIX socket). As a result it has to blindly set FD_PASS feature based solely on info in the SocketAddress struct, such as address type.

Unfortunately libvirt wishes to use FD passing to provide the UNIX domain socket listener, and as a result the FD_PASS feature is no longer set, which breaks vhostuser's checks, despite the fact that FD passing will in fact work later.

This unconditionally sets FD_PASS feature for any socket address which has type==fd. Thus will be wrong if the passed in FD was not a UNIX socket, but if an attempt is later made to use FD passing we'll still get an error reported by the QIOChannelSocket class. So the effective of setting the chardev FD_PASS feature early is merely to delay error reporting.

Quoted 1 line

Could you query with getsockopt or getsockname in tcp_chr_connect?

That potentially executes asynchronously in the backend and vhostuser checks it when it first resolves the chardev ID. IOW the point where vhostuser checks, all we have is the SocketAddress struct - in some case we might be lucky & have connected, but we can't assume it :-(

Previously vhostuser poked directly in the QemuOpts for the chardev which I changed to this feature based approach in

commit 0a73336d96397c80881219d080518fac6f1ecacb
Author: Daniel P. Berrange <[email protected]>
Date: Fri Oct 7 13:18:34 2016 +0100

net: don't poke at chardev internal QemuOpts

This whole mess is merely so that vhostuser can print out an error message if you point it to a chardev that isn't a UNIX socket. If anything I'm half inclined to just delete that upfront error check entirely.

Regards,
Daniel
--

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi

Quoted 25 lines · Daniel P. Berrangé

Quoted 24 lines · Paolo Bonzini

Quoted 22 lines · Daniel P. Berrangé

The vhostuser network backend requires the chardev it is using to have the FD passing feature. It checks this upfront when initializing the network backend and reports an error if not set.

The socket chardev has to set the FD_PASS feature during early initialization to satisfy the vhostuser backend, and at this point the socket has not been initialized. It is thus unable to do a live check on the socket to see if it supports FD passing (aka is a UNIX socket). As a result it has to blindly set FD_PASS feature based solely on info in the SocketAddress struct, such as address type.

Unfortunately libvirt wishes to use FD passing to provide the UNIX domain socket listener, and as a result the FD_PASS feature is no longer set, which breaks vhostuser's checks, despite the fact that FD passing will in fact work later.

This unconditionally sets FD_PASS feature for any socket address which has type==fd. Thus will be wrong if the passed in FD was not a UNIX socket, but if an attempt is later made to use FD passing we'll still get an error reported by the QIOChannelSocket class. So the effective of setting the chardev FD_PASS feature early is merely to delay error reporting.

Quoted 6 lines

Quoted 1 line

Could you query with getsockopt or getsockname in tcp_chr_connect?

That potentially executes asynchronously in the backend and vhostuser checks it when it first resolves the chardev ID. IOW the point where vhostuser checks, all we have is the SocketAddress struct - in some case we might be lucky & have connected, but we can't assume it :-(

In which case is it executed asynchronously?

Chardev creation is happening before vhost-user net, qemu_chardev_new() and qemu_char_open() calls qio_channel_socket_connect_sync().

Quoted 37 lines · Marc-André Lureau

Hi

Quoted 25 lines · Daniel P. Berrangé

Quoted 24 lines · Paolo Bonzini

Quoted 22 lines · Daniel P. Berrangé

The vhostuser network backend requires the chardev it is using to have the FD passing feature. It checks this upfront when initializing the network backend and reports an error if not set.

The socket chardev has to set the FD_PASS feature during early initialization to satisfy the vhostuser backend, and at this point the socket has not been initialized. It is thus unable to do a live check on the socket to see if it supports FD passing (aka is a UNIX socket). As a result it has to blindly set FD_PASS feature based solely on info in the SocketAddress struct, such as address type.

Unfortunately libvirt wishes to use FD passing to provide the UNIX domain socket listener, and as a result the FD_PASS feature is no longer set, which breaks vhostuser's checks, despite the fact that FD passing will in fact work later.

This unconditionally sets FD_PASS feature for any socket address which has type==fd. Thus will be wrong if the passed in FD was not a UNIX socket, but if an attempt is later made to use FD passing we'll still get an error reported by the QIOChannelSocket class. So the effective of setting the chardev FD_PASS feature early is merely to delay error reporting.

Quoted 6 lines

Quoted 1 line

Could you query with getsockopt or getsockname in tcp_chr_connect?

That potentially executes asynchronously in the backend and vhostuser checks it when it first resolves the chardev ID. IOW the point where vhostuser checks, all we have is the SocketAddress struct - in some case we might be lucky & have connected, but we can't assume it :-(

In which case is it executed asynchronously?

Quoted 3 lines

Chardev creation is happening before vhost-user net, qemu_chardev_new() and qemu_char_open() calls qio_channel_socket_connect_sync().

The qmp_chardev_open_socket() method is not guaranteed to complete synchronously. If the "reconnect" property is set everything happens asynchronously, so we're not able to set the feature flag by the time qemu_char_open() returns.