[Qemu-commits] [qemu/qemu] d88610: chardev: comment details for CLOSED event

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: d88610111b40bca19925ece0fa81710d425725a8
      
https://github.com/qemu/qemu/commit/d88610111b40bca19925ece0fa81710d425725a8
  Author: Peter Xu <address@hidden>
  Date:   2018-06-30 (Sat, 30 Jun 2018)

  Changed paths:
    M include/chardev/char.h

  Log Message:
  -----------
  chardev: comment details for CLOSED event

It was unclear before on what does the CLOSED event mean.  Meanwhile we
add a TODO to fix up the CLOSED event in the future when the in/out
ports are different for a chardev.

CC: Paolo Bonzini <address@hidden>
CC: "Marc-André Lureau" <address@hidden>
CC: Stefan Hajnoczi <address@hidden>
CC: Markus Armbruster <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
Signed-off-by: Peter Xu <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 40687eb741a974c47326ef3cf7f6b25cc0680552
      
https://github.com/qemu/qemu/commit/40687eb741a974c47326ef3cf7f6b25cc0680552
  Author: Peter Xu <address@hidden>
  Date:   2018-06-30 (Sat, 30 Jun 2018)

  Changed paths:
    M monitor.c

  Log Message:
  -----------
  monitor: rename *_pop_one to *_pop_any

The old names are confusing since both of the old functions are popping
an item from multiple queues rather than a single queue.  In that
sense, *_pop_any() suites better than *_pop_one().

Since at it, touch up the function monitor_qmp_response_pop_any() a bit
to let the callers pass in a QMPResponse struct instead of returning a
struct.  Change the return value to boolean to mark whether we have
popped a valid response instead.

Suggested-by: Markus Armbruster <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Signed-off-by: Peter Xu <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: c73a843b4a822dc0bd5df5ef38dbd681dd96ad25
      
https://github.com/qemu/qemu/commit/c73a843b4a822dc0bd5df5ef38dbd681dd96ad25
  Author: Peter Xu <address@hidden>
  Date:   2018-06-30 (Sat, 30 Jun 2018)

  Changed paths:
    M monitor.c

  Log Message:
  -----------
  monitor: flush qmp responses when CLOSED

Previously we clean up the queues when we got CLOSED event.  It was used
to make sure we won't send leftover replies/events of a old client to a
new client which makes perfect sense. However this will also drop the
replies/events even if the output port of the previous chardev backend
is still open, which can lead to missing of the last replies/events.
Now this patch does an extra operation to flush the response queue
before cleaning up.

In most cases, a QMP session will be based on a bidirectional channel (a
TCP port, for example, we read/write to the same socket handle), so in
port and out port of the backend chardev are fundamentally the same
port. In these cases, it does not really matter much on whether we'll
flush the response queue since flushing will fail anyway.  However there
can be cases where in & out ports of the QMP monitor's backend chardev
are separated.  Here is an example:

  cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands

In this case, the backend is fd-typed, and it is connected to stdio
where in port is stdin and out port is stdout.  Now if we drop all the
events on the response queue then filter_command process might miss some
events that it might expect.  The thing is that, when stdin closes,
stdout might still be there alive!

In practice, I encountered SHUTDOWN event missing when running test with
iotest 087 with Out-Of-Band enabled.  Here is one of the ways that this
can happen (after "quit" command is executed and QEMU quits the main
loop):

1. [main thread] QEMU queues a SHUTDOWN event into response queue.

2. "cat" terminates (to distinguish it from the animal, I quote it).

3. [monitor iothread] QEMU's monitor iothread reads EOF from stdin.

4. [monitor iothread] QEMU's monitor iothread calls the CLOSED event
   hook for the monitor, which will destroy the response queue of the
   monitor, then the SHUTDOWN event is dropped.

5. [main thread] QEMU's main thread cleans up the monitors in
   monitor_cleanup().  When trying to flush pending responses, it sees
   nothing.  SHUTDOWN is lost forever.

Note that before the monitor iothread was introduced, step [4]/[5] could
never happen since the main loop was the only place to detect the EOF
event of stdin and run the CLOSED event hooks.  Now things can happen in
parallel in the iothread.

Without this patch, iotest 087 will have ~10% chance to miss the
SHUTDOWN event and fail when with Out-Of-Band enabled:

  --- /home/peterx/git/qemu/tests/qemu-iotests/087.out
  +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad
  @@ -8,7 +8,6 @@
  {"return": {}}
  {"error": {"class": "GenericError", "desc": "'node-name' must be
  specified for the root node"}}
  {"return": {}}
  -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}

  === Duplicate ID ===
  @@ -53,7 +52,6 @@
  {"return": {}}
  {"return": {}}
  {"return": {}}

  -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}

This patch fixes the problem.

Fixes: 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)
Suggested-by: Markus Armbruster <address@hidden>
Signed-off-by: Peter Xu <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
[Commit message and a comment touched up]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: cbc4ae2d1a9f4a9fd6c2f5000d3845f6d28cccc1
      
https://github.com/qemu/qemu/commit/cbc4ae2d1a9f4a9fd6c2f5000d3845f6d28cccc1
  Author: Peter Xu <address@hidden>
  Date:   2018-06-30 (Sat, 30 Jun 2018)

  Changed paths:
    M tests/qemu-iotests/060
    M tests/qemu-iotests/060.out

  Log Message:
  -----------
  tests: iotests: drop some stderr line

In my Out-Of-Band test, "check -qcow2 060" fail with this:

  --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
  +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
  @@ -427,8 +427,8 @@
  QMP_VERSION
  {"return": {}}
  qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1
  index: 0); further non-fatal corruption events will be suppressed
  -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": 
"BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset 
0x2a2a2a0
  0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
  read failed: Input/output error
  +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": 
"BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset 
0x2a2a2a0
  0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
  {"return": ""}
  {"return": {}}
  {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
  "event": "SHUTDOWN", "data": {"guest": false}}

The order of the event and the in/out error line is swapped.  I didn't
dig up the reason, but AFAIU what we want to verify is the event rather
than stderr.  Let's drop the stderr line directly for this test.

Signed-off-by: Peter Xu <address@hidden>
Message-Id: <address@hidden>
[Commit message touched up]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 4bfa7974d90a0cbad29c0a27334d02cbd37bb23d
      
https://github.com/qemu/qemu/commit/4bfa7974d90a0cbad29c0a27334d02cbd37bb23d
  Author: Peter Xu <address@hidden>
  Date:   2018-06-30 (Sat, 30 Jun 2018)

  Changed paths:
    M docs/devel/qapi-code-gen.txt

  Log Message:
  -----------
  docs: mention shared state protect for OOB

Out-Of-Band handlers need to protect shared state if there is any.
Mention it in the document.  Meanwhile, touch up some other places too,
either with better English, or reordering of bullets.

Suggested-by: Markus Armbruster <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Signed-off-by: Peter Xu <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: e3800998e66c13b24d8cc8a06fdcc8d03cd408fc
      
https://github.com/qemu/qemu/commit/e3800998e66c13b24d8cc8a06fdcc8d03cd408fc
  Author: Peter Maydell <address@hidden>
  Date:   2018-06-30 (Sat, 30 Jun 2018)

  Changed paths:
    M docs/devel/qapi-code-gen.txt
    M include/chardev/char.h
    M monitor.c
    M tests/qemu-iotests/060
    M tests/qemu-iotests/060.out

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/armbru/tags/pull-monitor-2018-06-30' 
into staging

Monitor patches for 2018-06-30

# gpg: Signature made Sat 30 Jun 2018 17:22:12 BST
# gpg:                using RSA key 3870B400EB918653
# gpg: Good signature from "Markus Armbruster <address@hidden>"
# gpg:                 aka "Markus Armbruster <address@hidden>"
# Primary key fingerprint: 354B C8B3 D7EB 2A6B 6867  4E5F 3870 B400 EB91 8653

* remotes/armbru/tags/pull-monitor-2018-06-30:
  docs: mention shared state protect for OOB
  tests: iotests: drop some stderr line
  monitor: flush qmp responses when CLOSED
  monitor: rename *_pop_one to *_pop_any
  chardev: comment details for CLOSED event

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/6f4fa0998fd1...e3800998e66c
      **NOTE:** This service been marked for deprecation: 
https://developer.github.com/changes/2018-04-25-github-services-deprecation/

      Functionality will be removed from GitHub.com on January 31st, 2019.