| Message ID | 20180704111118.7241-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: unconditionally set FD_PASS feature for socket type=fd | expand |
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); > } > >
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
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 :| >
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
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?
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); }
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(-)