Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> Previously we cleanup the queues when we got CLOSED event.  It was used

we clean up

> to make sure we won't leftover replies/events of a old client to a new

we won't send leftover replies/events of an old client

> client.  Now this patch postpones that until OPENED.

What if OPENED never comes?

> In most cases, it does not really matter much since either way will make
> sure that the new client won't receive unexpected old events/responses.
> However there can be a very rare race condition with the old way when we
> use QMP with stdio and pipelines (which is quite common in iotests).
> The problem is that, we can easily have something like this in scripts:
>
>   cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands
>
> In most cases, a QMP session will be based on a bidirectional
> channel (read/write to a TCP port, for example), so IN and OUT are
> sharing some fundamentally same thing.  However that's untrue for pipes

Suggest "are fundamentally the same thing".

> above - the IN is the "cat" program, while the "OUT" is directed to the
> "filter_commands" which is actually another process.

Regarding 'IN is the "cat" program': a pipe is not a program.  Suggest
'IN is connected to "cat", while OUT is connected to "filter_commands",
which are separate processes.

> Now when we received the CLOSED event, we cleanup the queues (including

we clean up

> the QMP response queue).  That means, if we kill/stop the "cat" process
> faster than the filter_commands process, the filter_commands process is
> possible to miss some responses/events that should belong to it.

I'm not sure I understand the error scenario.  Can you explain it in
more concrete terms?  Like "cat terminates, QEMU reads EOF, QEMU does
this, QEMU does that, oops, it shouldn't have done that".

> In practise, I encountered a very strange SHUTDOWN event missing when

practice

May I suggest use of a spell checker?  ;)

> running test with iotest 087.  Without this patch, iotest 078 will have

087 or 078?

> ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band
> enabled:
>
> 087 8s ... - output mismatch (see 087.out.bad)