On Thu, Feb 22, 2018 at 06:01:19PM +0800, Peter Xu wrote:
> On Wed, Feb 21, 2018 at 04:00:07PM +0000, Stefan Hajnoczi wrote:
> > On Wed, Jan 24, 2018 at 01:39:43PM +0800, Peter Xu wrote:
> > > @@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags)
> > > }
> > >
> > > if (monitor_is_qmp(mon)) {
> > > - qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> > > monitor_qmp_read,
> > > - monitor_qmp_event, NULL, mon, NULL,
> > > true);
> > > qemu_chr_fe_set_echo(&mon->chr, true);
> > > json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> > > + if (mon->use_io_thr) {
> > > + /*
> > > + * It's possible that we already have an IOWatchPoll
> > > + * registered for the Chardev during chardev_init_func().
> >
> > When does this happen?
> >
> > This seems like a hack that breaks when certain -chardev options are
> > used. For example, what happens if the chardev is a TCP connection with
> > reconnect=5. In that case the socket will be connecting asynchronously
> > and we cannot just remove the fd watch.
> >
> > How does this interact with TCP listen chardevs? It looks like the
> > listener socket uses the main loop (see tcp_chr_disconnect()).
> >
> > I'm worried that the chardev layer isn't thread-safe and you haven't
> > added anything to protect it or at least refuse to run in unsafe
> > conditions.
>
> Indeed, I did some more reading and noticed that the TCP typed chardev
> is really special.
>
> Firstly there can be the QIO thread that handles sync connecting when
> "reconnect" is setup (I don't really understand why we only need the
> threads when reconnect != 0, but anyway, I'll just assume we need the
> threads). It's done in qmp_chardev_open_socket().
>
> Secondly, TCP can support TLS or TELNET (tcp_chr_new_client() handles
> the main logic of it), so there can be actually more than one GSource
> created for a single TCP chardev. Meanwhile, the
> chr_update_read_handler() calls never handles the re-setup of those
> special GSources (TLS/TELNET), only the common GSource of TCP stream
> read/write.
>
> And the whole TCP channel is based on QIO stuff, which means I need to
> add non-default context support to QIO stuff too... That's mostly
> about qio_channel_add_watch(). I may need to pass in context
> information, and switch to GSource for that function instead of the
> old tags, just like what I did to chardev in general.
>
> I'll think about these. I may possibly need some pre-requisite and
> separated patches to fix existing problems before going on with OOB
> again.
>
> This is the worst thing I'd like to see - "surprises". :(Yes, this feature is more involved than anyone thought at the beginning. Stefan
signature.asc
Description: PGP signature
