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 <marcandre.lureau@redhat.com>
---
 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);
+    }
     if (s->do_nodelay) {
         qio_channel_set_delay(s->ioc, false);
     }
@@ -996,11 +999,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
         error_setg(errp, "'reconnect' option is incompatible with 'fd'");
         goto error;
     }
+
     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) {
-        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
-    }
 
     /* be isn't opened until we get a connection */
     *be_opened = false;
-- 
2.18.0.129.ge3331758f1

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 <marcandre.lureau@redhat.com>
> ---
>  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.

>      /* be isn't opened until we get a connection */
>      *be_opened = false;
> -- 
> 2.18.0.129.ge3331758f1
> 

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

On Tue, Jul 17, 2018 at 3:01 PM, Daniel P. Berrangé <berrange@redhat.com> 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 <marcandre.lureau@redhat.com>
>> ---
>>  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?

>
>>      /* be isn't opened until we get a connection */
>>      *be_opened = false;
>> --
>> 2.18.0.129.ge3331758f1
>>
>
> 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

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

Hi

On Tue, Jul 17, 2018 at 3:11 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 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é <berrange@redhat.com> 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 <marcandre.lureau@redhat.com>
>> >> ---
>> >>  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.

It sounds less optimal than what I proposed, but I don't mind.

What do you think of the first 2 patches?


-- 
Marc-André Lureau