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é <address@hidden> 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 <address@hidden>
>   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