Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done
On Thu, Mar 08, 2018 at 01:31:43PM +, Daniel P. Berrangé wrote:
> On Thu, Mar 08, 2018 at 07:42:13PM +0800, Peter Xu wrote:
> > On Thu, Mar 08, 2018 at 10:13:59AM +, Daniel P. Berrangé wrote:
> > > On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> > > > On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
> > > >
> > > > [...]
> > > >
> > > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > > > index bd40864f87..997c70dd7d 100644
> > > > > > --- a/chardev/char-socket.c
> > > > > > +++ b/chardev/char-socket.c
> > > > > > @@ -31,6 +31,7 @@
> > > > > > #include "qemu/option.h"
> > > > > > #include "qapi/error.h"
> > > > > > #include "qapi/clone-visitor.h"
> > > > > > +#include "sysemu/sysemu.h"
> > > > > >
> > > > > > #include "chardev/char-io.h"
> > > > > >
> > > > > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > > > > Error *err = NULL;
> > > > > > gchar *name;
> > > > > >
> > > > > > +if (!machine_init_done) {
> > > > > > +/* This will be postponed to machine_done notifier */
> > > > > > +return;
> > > > > > +}
> > > > > > +
> > > > > > if (s->is_listen) {
> > > > > > tioc = qio_channel_tls_new_server(
> > > > > > s->ioc, s->tls_creds,
> > > > > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev
> > > > > > *chr)
> > > > > > tcp_chr_connect_async(chr);
> > > > > > }
> > > > > >
> > > > > > +if (s->tls_creds) {
> > > > > > +tcp_chr_tls_init(chr);
> > > > > > +}
> > > > >
> > > > > This looks questionable - AFAICT, there's no guarantee we have any
> > > > > client connection active when the machine dnoe hook runs. Only if
> > > > > the chardev is set in client mode, and reconnect_time is *not* set,
> > > > > but this seems to be run unconditionally.
> > > >
> > > > You are right. Thanks for spotting that.
> > > >
> > > > Then how about this? It's a bit ugly, but I think it should be safe:
> > >
> > > Is it perhaps not possible to just check if 's->ioc' is non-NULL
> > > in the tcp_chr_machine_done_hook for your original patch ?
> >
> > In tcp_chr_new_client() I see that s->ioc will be set no matter what,
> > and that function can even be called without TLS enabled I think.
> > Does that mean only check against s->ioc would not be enough?
>
> I mean like this:
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index a1966aae51..19e3193817 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -723,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> Error *err = NULL;
> gchar *name;
>
> +if (!machine_init_done) {
> +/* This will be postponed to machine_done notifier */
> +return;
> +}
> +
> if (s->is_listen) {
> tioc = qio_channel_tls_new_server(
> s->ioc, s->tls_creds,
> @@ -1146,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> tcp_chr_connect_async(chr);
> }
>
> +if (s->ioc && s->tls_creds) {
> +tcp_chr_tls_init(chr);
> +}
> +
> return 0;
> }
>
>
>
> s->ioc will only be != NULL, if some codepath in qemu_chr_parse_socket
I guess here you mean qmp_chardev_open_socket().
> ended up calling tcp_chr_new_client(). If that happened we will have
> skipped setup of TLS, so calling tcp_chr_tls_init() based on
> s->ioc && s->tls_creds feels right to me.
Yes, it looks right to me too. :)
I'll post a complete patch soon with your authorship.
Thanks!
--
Peter Xu
Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done
On Thu, Mar 08, 2018 at 07:42:13PM +0800, Peter Xu wrote:
> On Thu, Mar 08, 2018 at 10:13:59AM +, Daniel P. Berrangé wrote:
> > On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> > > On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
> > >
> > > [...]
> > >
> > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > > index bd40864f87..997c70dd7d 100644
> > > > > --- a/chardev/char-socket.c
> > > > > +++ b/chardev/char-socket.c
> > > > > @@ -31,6 +31,7 @@
> > > > > #include "qemu/option.h"
> > > > > #include "qapi/error.h"
> > > > > #include "qapi/clone-visitor.h"
> > > > > +#include "sysemu/sysemu.h"
> > > > >
> > > > > #include "chardev/char-io.h"
> > > > >
> > > > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > > > Error *err = NULL;
> > > > > gchar *name;
> > > > >
> > > > > +if (!machine_init_done) {
> > > > > +/* This will be postponed to machine_done notifier */
> > > > > +return;
> > > > > +}
> > > > > +
> > > > > if (s->is_listen) {
> > > > > tioc = qio_channel_tls_new_server(
> > > > > s->ioc, s->tls_creds,
> > > > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev
> > > > > *chr)
> > > > > tcp_chr_connect_async(chr);
> > > > > }
> > > > >
> > > > > +if (s->tls_creds) {
> > > > > +tcp_chr_tls_init(chr);
> > > > > +}
> > > >
> > > > This looks questionable - AFAICT, there's no guarantee we have any
> > > > client connection active when the machine dnoe hook runs. Only if
> > > > the chardev is set in client mode, and reconnect_time is *not* set,
> > > > but this seems to be run unconditionally.
> > >
> > > You are right. Thanks for spotting that.
> > >
> > > Then how about this? It's a bit ugly, but I think it should be safe:
> >
> > Is it perhaps not possible to just check if 's->ioc' is non-NULL
> > in the tcp_chr_machine_done_hook for your original patch ?
>
> In tcp_chr_new_client() I see that s->ioc will be set no matter what,
> and that function can even be called without TLS enabled I think.
> Does that mean only check against s->ioc would not be enough?
I mean like this:
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index a1966aae51..19e3193817 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -723,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
Error *err = NULL;
gchar *name;
+if (!machine_init_done) {
+/* This will be postponed to machine_done notifier */
+return;
+}
+
if (s->is_listen) {
tioc = qio_channel_tls_new_server(
s->ioc, s->tls_creds,
@@ -1146,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
tcp_chr_connect_async(chr);
}
+if (s->ioc && s->tls_creds) {
+tcp_chr_tls_init(chr);
+}
+
return 0;
}
s->ioc will only be != NULL, if some codepath in qemu_chr_parse_socket
ended up calling tcp_chr_new_client(). If that happened we will have
skipped setup of TLS, so calling tcp_chr_tls_init() based on
s->ioc && s->tls_creds feels right to me.
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 v2 9/9] chardev: tcp: postpone TLS work until machine done
On Thu, Mar 08, 2018 at 10:13:59AM +, Daniel P. Berrangé wrote:
> On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> > On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
> >
> > [...]
> >
> > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > index bd40864f87..997c70dd7d 100644
> > > > --- a/chardev/char-socket.c
> > > > +++ b/chardev/char-socket.c
> > > > @@ -31,6 +31,7 @@
> > > > #include "qemu/option.h"
> > > > #include "qapi/error.h"
> > > > #include "qapi/clone-visitor.h"
> > > > +#include "sysemu/sysemu.h"
> > > >
> > > > #include "chardev/char-io.h"
> > > >
> > > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > > Error *err = NULL;
> > > > gchar *name;
> > > >
> > > > +if (!machine_init_done) {
> > > > +/* This will be postponed to machine_done notifier */
> > > > +return;
> > > > +}
> > > > +
> > > > if (s->is_listen) {
> > > > tioc = qio_channel_tls_new_server(
> > > > s->ioc, s->tls_creds,
> > > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev
> > > > *chr)
> > > > tcp_chr_connect_async(chr);
> > > > }
> > > >
> > > > +if (s->tls_creds) {
> > > > +tcp_chr_tls_init(chr);
> > > > +}
> > >
> > > This looks questionable - AFAICT, there's no guarantee we have any
> > > client connection active when the machine dnoe hook runs. Only if
> > > the chardev is set in client mode, and reconnect_time is *not* set,
> > > but this seems to be run unconditionally.
> >
> > You are right. Thanks for spotting that.
> >
> > Then how about this? It's a bit ugly, but I think it should be safe:
>
> Is it perhaps not possible to just check if 's->ioc' is non-NULL
> in the tcp_chr_machine_done_hook for your original patch ?
In tcp_chr_new_client() I see that s->ioc will be set no matter what,
and that function can even be called without TLS enabled I think.
Does that mean only check against s->ioc would not be enough?
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done
On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
>
> [...]
>
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index bd40864f87..997c70dd7d 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -31,6 +31,7 @@
> > > #include "qemu/option.h"
> > > #include "qapi/error.h"
> > > #include "qapi/clone-visitor.h"
> > > +#include "sysemu/sysemu.h"
> > >
> > > #include "chardev/char-io.h"
> > >
> > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > Error *err = NULL;
> > > gchar *name;
> > >
> > > +if (!machine_init_done) {
> > > +/* This will be postponed to machine_done notifier */
> > > +return;
> > > +}
> > > +
> > > if (s->is_listen) {
> > > tioc = qio_channel_tls_new_server(
> > > s->ioc, s->tls_creds,
> > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > > tcp_chr_connect_async(chr);
> > > }
> > >
> > > +if (s->tls_creds) {
> > > +tcp_chr_tls_init(chr);
> > > +}
> >
> > This looks questionable - AFAICT, there's no guarantee we have any
> > client connection active when the machine dnoe hook runs. Only if
> > the chardev is set in client mode, and reconnect_time is *not* set,
> > but this seems to be run unconditionally.
>
> You are right. Thanks for spotting that.
>
> Then how about this? It's a bit ugly, but I think it should be safe:
Is it perhaps not possible to just check if 's->ioc' is non-NULL
in the tcp_chr_machine_done_hook for your original patch ?
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bd40864f87..b4686fd23f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -31,6 +31,7 @@
> #include "qemu/option.h"
> #include "qapi/error.h"
> #include "qapi/clone-visitor.h"
> +#include "sysemu/sysemu.h"
>
> #include "chardev/char-io.h"
>
> @@ -51,6 +52,11 @@ typedef struct {
> QIONetListener *listener;
> GSource *hup_source;
> QCryptoTLSCreds *tls_creds;
> +/*
> + * This should only be used once - when we want to setup TLS for
> + * the session but we need to wait until machine init done.
> + */
> +bool tls_need_postponed_init;
> int connected;
> int max_size;
> int do_telnetopt;
> @@ -791,7 +797,15 @@ static int tcp_chr_new_client(Chardev *chr,
> QIOChannelSocket *sioc)
> }
>
> if (s->tls_creds) {
> -tcp_chr_tls_init(chr);
> +if (machine_init_done) {
> +tcp_chr_tls_init(chr);
> +} else {
> +/*
> + * Postpone to machine init done since we need the correct
> + * context to setup the TLS handshake.
> + */
> +s->tls_need_postponed_init = true;
> +}
> } else {
> if (s->do_telnetopt) {
> tcp_chr_telnet_init(chr);
> @@ -1145,6 +1159,11 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> tcp_chr_connect_async(chr);
> }
>
> +if (s->tls_need_postponed_init) {
> +assert(s->tls_creds);
> +tcp_chr_tls_init(chr);
> +}
> +
> return 0;
> }
>
> Thanks,
>
> --
> Peter Xu
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 v2 9/9] chardev: tcp: postpone TLS work until machine done
On Wed, Mar 07, 2018 at 04:06:53PM +0100, Paolo Bonzini wrote: > On 07/03/2018 13:40, Daniel P. Berrangé wrote: > > On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote: > >> On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote: > >>> TLS handshake may create background GSource tasks, while we won't know > >>> the correct GMainContext until the whole chardev (including frontend) > >>> inited. Let's postpone the initial TLS handshake until machine done. > >>> > >>> For dynamically created tcp chardev, we don't postpone that by checking > >>> the init_machine_done variable. > >> > >> Not sure I see the need for this one - we've already postponed the > >> acceptance of a client in the patch 7. > > > > Opps, meant to remove this comment, in favour of the later comment - ignore > > this bit. > > Since time is ticking for soft freeze, I'll queue the series without > this patch. Thanks Paolo. Note that Dan's pull is still not merged yet, and this series will need that one. I'll try to see whether I can prepare a good version for the last TLS patch before you start to test your next pull request. -- Peter Xu
Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done
On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
[...]
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index bd40864f87..997c70dd7d 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -31,6 +31,7 @@
> > #include "qemu/option.h"
> > #include "qapi/error.h"
> > #include "qapi/clone-visitor.h"
> > +#include "sysemu/sysemu.h"
> >
> > #include "chardev/char-io.h"
> >
> > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > Error *err = NULL;
> > gchar *name;
> >
> > +if (!machine_init_done) {
> > +/* This will be postponed to machine_done notifier */
> > +return;
> > +}
> > +
> > if (s->is_listen) {
> > tioc = qio_channel_tls_new_server(
> > s->ioc, s->tls_creds,
> > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > tcp_chr_connect_async(chr);
> > }
> >
> > +if (s->tls_creds) {
> > +tcp_chr_tls_init(chr);
> > +}
>
> This looks questionable - AFAICT, there's no guarantee we have any
> client connection active when the machine dnoe hook runs. Only if
> the chardev is set in client mode, and reconnect_time is *not* set,
> but this seems to be run unconditionally.
You are right. Thanks for spotting that.
Then how about this? It's a bit ugly, but I think it should be safe:
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bd40864f87..b4686fd23f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -31,6 +31,7 @@
#include "qemu/option.h"
#include "qapi/error.h"
#include "qapi/clone-visitor.h"
+#include "sysemu/sysemu.h"
#include "chardev/char-io.h"
@@ -51,6 +52,11 @@ typedef struct {
QIONetListener *listener;
GSource *hup_source;
QCryptoTLSCreds *tls_creds;
+/*
+ * This should only be used once - when we want to setup TLS for
+ * the session but we need to wait until machine init done.
+ */
+bool tls_need_postponed_init;
int connected;
int max_size;
int do_telnetopt;
@@ -791,7 +797,15 @@ static int tcp_chr_new_client(Chardev *chr,
QIOChannelSocket *sioc)
}
if (s->tls_creds) {
-tcp_chr_tls_init(chr);
+if (machine_init_done) {
+tcp_chr_tls_init(chr);
+} else {
+/*
+ * Postpone to machine init done since we need the correct
+ * context to setup the TLS handshake.
+ */
+s->tls_need_postponed_init = true;
+}
} else {
if (s->do_telnetopt) {
tcp_chr_telnet_init(chr);
@@ -1145,6 +1159,11 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
tcp_chr_connect_async(chr);
}
+if (s->tls_need_postponed_init) {
+assert(s->tls_creds);
+tcp_chr_tls_init(chr);
+}
+
return 0;
}
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done
On 07/03/2018 13:40, Daniel P. Berrangé wrote:
> On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
>> On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
>>> TLS handshake may create background GSource tasks, while we won't know
>>> the correct GMainContext until the whole chardev (including frontend)
>>> inited. Let's postpone the initial TLS handshake until machine done.
>>>
>>> For dynamically created tcp chardev, we don't postpone that by checking
>>> the init_machine_done variable.
>>
>> Not sure I see the need for this one - we've already postponed the
>> acceptance of a client in the patch 7.
>
> Opps, meant to remove this comment, in favour of the later comment - ignore
> this bit.
Since time is ticking for soft freeze, I'll queue the series without
this patch.
Paolo
>>
>>>
>>> Signed-off-by: Peter Xu
>>> ---
>>> chardev/char-socket.c | 10 ++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index bd40864f87..997c70dd7d 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -31,6 +31,7 @@
>>> #include "qemu/option.h"
>>> #include "qapi/error.h"
>>> #include "qapi/clone-visitor.h"
>>> +#include "sysemu/sysemu.h"
>>>
>>> #include "chardev/char-io.h"
>>>
>>> @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
>>> Error *err = NULL;
>>> gchar *name;
>>>
>>> +if (!machine_init_done) {
>>> +/* This will be postponed to machine_done notifier */
>>> +return;
>>> +}
>>> +
>>> if (s->is_listen) {
>>> tioc = qio_channel_tls_new_server(
>>> s->ioc, s->tls_creds,
>>> @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
>>> tcp_chr_connect_async(chr);
>>> }
>>>
>>> +if (s->tls_creds) {
>>> +tcp_chr_tls_init(chr);
>>> +}
>>
>> This looks questionable - AFAICT, there's no guarantee we have any
>> client connection active when the machine dnoe hook runs. Only if
>> the chardev is set in client mode, and reconnect_time is *not* set,
>> but this seems to be run unconditionally.
>>
>>
>> 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
>> :|
>>
>
> Regards,
> Daniel
>
Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done
On Wed, Mar 07, 2018 at 12:36:50PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
> > TLS handshake may create background GSource tasks, while we won't know
> > the correct GMainContext until the whole chardev (including frontend)
> > inited. Let's postpone the initial TLS handshake until machine done.
> >
> > For dynamically created tcp chardev, we don't postpone that by checking
> > the init_machine_done variable.
>
> Not sure I see the need for this one - we've already postponed the
> acceptance of a client in the patch 7.
Opps, meant to remove this comment, in favour of the later comment - ignore
this bit.
>
> >
> > Signed-off-by: Peter Xu
> > ---
> > chardev/char-socket.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index bd40864f87..997c70dd7d 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -31,6 +31,7 @@
> > #include "qemu/option.h"
> > #include "qapi/error.h"
> > #include "qapi/clone-visitor.h"
> > +#include "sysemu/sysemu.h"
> >
> > #include "chardev/char-io.h"
> >
> > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > Error *err = NULL;
> > gchar *name;
> >
> > +if (!machine_init_done) {
> > +/* This will be postponed to machine_done notifier */
> > +return;
> > +}
> > +
> > if (s->is_listen) {
> > tioc = qio_channel_tls_new_server(
> > s->ioc, s->tls_creds,
> > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > tcp_chr_connect_async(chr);
> > }
> >
> > +if (s->tls_creds) {
> > +tcp_chr_tls_init(chr);
> > +}
>
> This looks questionable - AFAICT, there's no guarantee we have any
> client connection active when the machine dnoe hook runs. Only if
> the chardev is set in client mode, and reconnect_time is *not* set,
> but this seems to be run unconditionally.
>
>
> 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 :|
>
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 v2 9/9] chardev: tcp: postpone TLS work until machine done
On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
> TLS handshake may create background GSource tasks, while we won't know
> the correct GMainContext until the whole chardev (including frontend)
> inited. Let's postpone the initial TLS handshake until machine done.
>
> For dynamically created tcp chardev, we don't postpone that by checking
> the init_machine_done variable.
Not sure I see the need for this one - we've already postponed the
acceptance of a client in the patch 7.
>
> Signed-off-by: Peter Xu
> ---
> chardev/char-socket.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bd40864f87..997c70dd7d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -31,6 +31,7 @@
> #include "qemu/option.h"
> #include "qapi/error.h"
> #include "qapi/clone-visitor.h"
> +#include "sysemu/sysemu.h"
>
> #include "chardev/char-io.h"
>
> @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> Error *err = NULL;
> gchar *name;
>
> +if (!machine_init_done) {
> +/* This will be postponed to machine_done notifier */
> +return;
> +}
> +
> if (s->is_listen) {
> tioc = qio_channel_tls_new_server(
> s->ioc, s->tls_creds,
> @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> tcp_chr_connect_async(chr);
> }
>
> +if (s->tls_creds) {
> +tcp_chr_tls_init(chr);
> +}
This looks questionable - AFAICT, there's no guarantee we have any
client connection active when the machine dnoe hook runs. Only if
the chardev is set in client mode, and reconnect_time is *not* set,
but this seems to be run unconditionally.
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 v2 9/9] chardev: tcp: postpone TLS work until machine done
TLS handshake may create background GSource tasks, while we won't know
the correct GMainContext until the whole chardev (including frontend)
inited. Let's postpone the initial TLS handshake until machine done.
For dynamically created tcp chardev, we don't postpone that by checking
the init_machine_done variable.
Signed-off-by: Peter Xu
---
chardev/char-socket.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bd40864f87..997c70dd7d 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -31,6 +31,7 @@
#include "qemu/option.h"
#include "qapi/error.h"
#include "qapi/clone-visitor.h"
+#include "sysemu/sysemu.h"
#include "chardev/char-io.h"
@@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
Error *err = NULL;
gchar *name;
+if (!machine_init_done) {
+/* This will be postponed to machine_done notifier */
+return;
+}
+
if (s->is_listen) {
tioc = qio_channel_tls_new_server(
s->ioc, s->tls_creds,
@@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
tcp_chr_connect_async(chr);
}
+if (s->tls_creds) {
+tcp_chr_tls_init(chr);
+}
+
return 0;
}
--
2.14.3
