[Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[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.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 17519ec589..b495d6a851 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -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);
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[Paolo Bonzini]

On 04/07/2018 13:11, Daniel P. Berrangé wrote:
> 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.

Could you query with getsockopt or getsockname in tcp_chr_connect?

Paolo

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 17519ec589..b495d6a851 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -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);
>      }
>  
> 

Re: [Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[Daniel P. Berrangé]

On Wed, Jul 04, 2018 at 01:53:03PM +0200, Paolo Bonzini wrote:
> On 04/07/2018 13:11, Daniel P. Berrangé wrote:
> > 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.
> 
> 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 <berrange@redhat.com>
  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 :|

Re: [Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[Marc-André Lureau]

Hi

On Wed, Jul 4, 2018 at 2:08 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Wed, Jul 04, 2018 at 01:53:03PM +0200, Paolo Bonzini wrote:
>> On 04/07/2018 13:11, Daniel P. Berrangé wrote:
>> > 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.
>>
>> 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().

thanks

>
> 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 <berrange@redhat.com>
>   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 :|
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[Daniel P. Berrangé]

On Mon, Jul 16, 2018 at 06:57:48PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 4, 2018 at 2:08 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Wed, Jul 04, 2018 at 01:53:03PM +0200, Paolo Bonzini wrote:
> >> On 04/07/2018 13:11, Daniel P. Berrangé wrote:
> >> > 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.
> >>
> >> 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().

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.


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 :|

Re: [Qemu-devel] [PATCH] chardev: unconditionally set FD_PASS feature for socket type=fd

[Marc-André Lureau]

Hi

On Mon, Jul 16, 2018 at 7:03 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jul 16, 2018 at 06:57:48PM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Wed, Jul 4, 2018 at 2:08 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > On Wed, Jul 04, 2018 at 01:53:03PM +0200, Paolo Bonzini wrote:
>> >> On 04/07/2018 13:11, Daniel P. Berrangé wrote:
>> >> > 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.
>> >>
>> >> 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().
>
> 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.

Shouldn't reconnect= and fd= arguments be incompatible? Does it make
sense to try a reconnect on the same fd?




-- 
Marc-André Lureau