Re: [Qemu-devel] monitor: enable OOB by default

[Peter Xu]

On Wed, Jun 27, 2018 at 10:35:15AM +0200, Markus Armbruster wrote:
> Markus Armbruster <address@hidden> writes:
> 
> > Another lose end: event COMMAND_DROPPED seems to lack test coverage.
> 
> Hmm, dropping commands serves to limit the request queue.  What limits
> the response queue?

As long as we have a request queue limitation, that'll somehow be
"part of" the limitation of response queue.  Since the real responses
(let's not consider the events first) should be no more than the
maximum QMP requests that we allow in the request queue (one response
for one request).  In that sense it seems fine to me.

> 
> Before OOB, the monitor read at most one command, and wrote its response
> with monitor_puts().
> 
> For input, this leaves queueing to the kernel: if the client sends
> commands faster than the server can execute them, eventually the kernel
> refuses to buffer more, and the client's send either blocks or fails
> with EAGAIN.
> 
> Output is a mess.  monitor_puts() uses an output buffer.  It tries to
> flush at newline.  Issues:
> 
> * If flushing runs into a partial write, the unwritten remainder remains
>   in the output buffer until the next newline.  That newline may take
>   its own sweet time to arrive.  Could even lead to deadlocks, where a
>   client awaits complete output before it sends more input.  Bug,
>   predates OOB, doesn't block this series.

True.  Though I noticed that we have a "hackish" line in
monitor_json_emitter_raw():

    qstring_append_chr(json, '\n');

So it seems that at least we should never encounter a deadlock, after
all there will always be a newline there. But I'd say I agree with you
on that it's at least not that "beautiful". :-)

> 
> * If the client fails to read, the output buffer can grow without bound.
>   Not a security issue; the client is trusted.  Just bad workmanship.

True.

> 
> OOB doesn't change this for monitors running in the main thread.  Only
> mux chardevs run there.
> 
> Aside: keeping special case code around just for mux is a really bad
> idea.  We need to get rid of it.

We should be running the same code path even for MUX-ed typed, right?
Do you mean to put MUX-ed typed handling onto iothreads as well when
you say "get rid of it"?

> 
> For monitors running in an I/O thread, we add another buffer: the
> response queue.  It's drained by monitor_qmp_bh_responder().  I guess
> that means the response queue is effectively bounded by timely draining.
> Correct?

I don't see a timely manner to flush it, but as long as we queue
anything (including events) onto the response queue, we'll poke the
bottom half (in monitor_json_emitter() we call qemu_bh_schedule()) so
we'll possibly drain the queue very soon, and there should be no
chance to have a stale message in that queue.

> 
> Buffering twice seems silly, but that could be addressed in follow-up
> patches.

Do you mean that we can write the response immediately into
Monitor.outbuf, then only flush it in iothread?  IMHO that's fine -
after all, the response queue, as mentioned above, should have a
natural restriction as well due to the request queue, then we won't
waste too much resources for that.  Meanwhile using a queue with QMP
response objects seems to be a bit more cleaner to me from design pov
(though I might be wrong).

Regards,

-- 
Peter Xu