Re: [Qemu-devel] [PATCH 3/3] char-socket: delay setting fd-pass feature until connected

[Daniel P . Berrangé]

On Tue, Jul 17, 2018 at 03:07:01PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jul 17, 2018 at 3:01 PM, Daniel P. Berrangé <address@hidden> wrote:
> > On Tue, Jul 17, 2018 at 02:52:39PM +0200, Marc-André Lureau wrote:
> >> Wait for QIO channel connection completion, and check the feature set
> >> by QIO. This fixes setting fd-pass chardev feature on
> >> SOCKET_ADDRESS_FD where fd has AF_UNIX.
> >>
> >> Signed-off-by: Marc-André Lureau <address@hidden>
> >> ---
> >>  chardev/char-socket.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> >> index 6daa8d003a..7387e632d4 100644
> >> --- a/chardev/char-socket.c
> >> +++ b/chardev/char-socket.c
> >> @@ -789,6 +789,9 @@ static int tcp_chr_new_client(Chardev *chr, 
> >> QIOChannelSocket *sioc)
> >>
> >>      qio_channel_set_blocking(s->ioc, false, NULL);
> >>
> >> +    if (qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
> >> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
> >> +    }
> >
> > I really don't like this approach as it has unpleasant async/race prone
> > semantics or the external users of the chardev.
> >
> > With the current approach they know that once the chardev is created,
> > the features have been initialized.
> >
> > With this approach, the features are only initialized once the client
> > connection has been completed, or once the server has started listening,
> > which may or may not be the case once the chardev constructor completes.
> 
> Ok, what about augmenting the feature set then?:
> 
> keep the current qemu_chr_set_feature() in contructor for
> ADDRESS_TYPE_UNIX, and add the feature in new_client() for
> ADDRESS_TYPE_FD?

IMHO that makes the behaviour even more non-deterministic for callers.

How about we just delete the entire feature concept from chardevs and remove
the check from the vhostuser network backend too.

We have lots of users of chardevs in QEMU and vhostuser is the only one
that has tried to do this sanity check for the "correct" chardev backend.
The other users just assume the user / mgmt app will configure a suitable
chardev backend.  The vhostuser sanity checks have just caused more problems
then they've solved.


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