[Qemu-devel] [PATCH 0/3] chardev: fix setting FD_PASS for socket type=fd

[Qemu-devel] [PATCH 0/3] chardev: fix setting FD_PASS for socket type=fd

[Marc-André Lureau]

Hi,

This is an alternative series from Daniel "[PATCH] chardev:
unconditionally set FD_PASS feature for socket type=fd".

Instead of unconditionally set FD_PASS feature for any socket address
which has type==fd, we can rely on QIO synchronized initialization and
feature check, as long as "reconnect" option is not provided. The
first 2 patches enforce this case is not supported, as it could give
unexpected or surprising results.

Marc-André Lureau (3):
  char-socket: correctly set has_reconnect when parsing QemuOpts
  char-socket: make 'fd' incompatible with 'reconnect'
  char-socket: delay setting fd-pass feature until connected

 chardev/char-socket.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.18.0.129.ge3331758f1

[Qemu-devel] [PATCH 1/3] char-socket: correctly set has_reconnect when parsing QemuOpts

[Marc-André Lureau]

qemu_chr_parse_socket() fills all ChardevSocket fields, but that
doesn't reflect correctly the arguments given with the options / on
the command line. "reconnect" takes a number as argument, and the
default value is 0, which doesn't help to identify the missing
option. The other arguments have default values that are less
problematic, leave them set by default for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index efbad6ee7c..8950f36317 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1111,7 +1111,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock->tn3270 = is_tn3270;
     sock->has_wait = true;
     sock->wait = is_waitconnect;
-    sock->has_reconnect = true;
+    sock->has_reconnect = qemu_opt_find(opts, "reconnect");
     sock->reconnect = reconnect;
     sock->tls_creds = g_strdup(tls_creds);
 
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH 1/3] char-socket: correctly set has_reconnect when parsing QemuOpts

[Daniel P. Berrangé]

On Tue, Jul 17, 2018 at 02:52:37PM +0200, Marc-André Lureau wrote:
> qemu_chr_parse_socket() fills all ChardevSocket fields, but that
> doesn't reflect correctly the arguments given with the options / on
> the command line. "reconnect" takes a number as argument, and the
> default value is 0, which doesn't help to identify the missing
> option. The other arguments have default values that are less
> problematic, leave them set by default for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index efbad6ee7c..8950f36317 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1111,7 +1111,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock->tn3270 = is_tn3270;
>      sock->has_wait = true;
>      sock->wait = is_waitconnect;
> -    sock->has_reconnect = true;
> +    sock->has_reconnect = qemu_opt_find(opts, "reconnect");
>      sock->reconnect = reconnect;
>      sock->tls_creds = g_strdup(tls_creds);

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

[Qemu-devel] [PATCH 2/3] char-socket: make 'fd' incompatible with 'reconnect'

[Marc-André Lureau]

A chardev socket created with the 'fd=' argument is not going to
handle reconnection properly by recycling the same fd (or not in a
supported way). Let's forbid this case.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 8950f36317..6daa8d003a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -992,6 +992,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
 
     s->addr = addr = socket_address_flatten(sock->addr);
 
+    if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) {
+        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) {
-- 
2.18.0.129.ge3331758f1

Re: [Qemu-devel] [PATCH 2/3] char-socket: make 'fd' incompatible with 'reconnect'

[Daniel P. Berrangé]

On Tue, Jul 17, 2018 at 02:52:38PM +0200, Marc-André Lureau wrote:
> A chardev socket created with the 'fd=' argument is not going to
> handle reconnection properly by recycling the same fd (or not in a
> supported way). Let's forbid this case.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-socket.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 8950f36317..6daa8d003a 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -992,6 +992,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
>  
>      s->addr = addr = socket_address_flatten(sock->addr);
>  
> +    if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) {
> +        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) {

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

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

[Marc-André Lureau]

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

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

[Daniel P. Berrangé]

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

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

[Marc-André Lureau]

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

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

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

[Marc-André Lureau]

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