[Qemu-commits] [qemu/qemu] 40196c: python/aqmp: add _session_guard()

[Peter Maydell]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 40196c23939758abc5300e85333e676196e3ba6d
      
https://github.com/qemu/qemu/commit/40196c23939758abc5300e85333e676196e3ba6d
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py

  Log Message:
  -----------
  python/aqmp: add _session_guard()

In _new_session, there's a fairly complex except clause that's used to
give semantic errors to callers of accept() and connect(). We need to
create a new two-step replacement for accept(), so factoring out this
piece of logic will be useful.

Bolster the comments and docstring here to try and demystify what's
going on in this fairly delicate piece of Python magic.

(If we were using Python 3.7+, this would be an @asynccontextmanager. We
don't have that very nice piece of magic, however, so this must take an
Awaitable to manage the Exception contexts properly. We pay the price
for platform compatibility.)

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-2-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 0ba4e76b23fed77d09be7f56da783ab3f0b2d497
      
https://github.com/qemu/qemu/commit/0ba4e76b23fed77d09be7f56da783ab3f0b2d497
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/legacy.py
    M python/qemu/aqmp/protocol.py
    M python/tests/protocol.py

  Log Message:
  -----------
  python/aqmp: rename 'accept()' to 'start_server_and_accept()'

Previously, I had a method named "accept()" that under-the-hood calls
bind(2), listen(2) *and* accept(2). I meant this as a simplification and
counterpart to the one-shot "connect()" method.

This is confusing to readers who expect accept() to mean *just*
accept(2). Since I need to split apart the "accept()" method into
multiple methods anyway (one of which strongly resembling accept(2)), it
feels pertinent to rename this method *now*.

Rename this all-in-one method "start_server_and_accept()" instead.

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-3-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 68a6cf3ffe3532c0655efbbf5910bd99a1b4a3fa
      
https://github.com/qemu/qemu/commit/68a6cf3ffe3532c0655efbbf5910bd99a1b4a3fa
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py
    M python/tests/protocol.py

  Log Message:
  -----------
  python/aqmp: remove _new_session and _establish_connection

These two methods attempted to entirely envelop the logic of
establishing a connection to a peer start to finish. However, we need to
break apart the incoming connection step into more granular steps. We
will no longer be able to reasonably constrain the logic inside of these
helper functions.

So, remove them - with _session_guard(), they no longer serve a real
purpose.

Although the public API doesn't change, the internal API does. Now that
there are no intermediary methods between e.g. connect() and
_do_connect(), there's no hook where the runstate is set. As a result,
the test suite changes a little to cope with the new semantics of
_do_accept() and _do_connect().

Lastly, take some pieces of the now-deleted docstrings and move
them up to the public interface level. They were a little more detailed,
and it won't hurt to keep them.

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-4-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 830e6fd36e2aef37b158a10dea6c3853ce43b20c
      
https://github.com/qemu/qemu/commit/830e6fd36e2aef37b158a10dea6c3853ce43b20c
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py

  Log Message:
  -----------
  python/aqmp: split _client_connected_cb() out as _incoming()

As part of disentangling the monolithic nature of _do_accept(), split
out the incoming callback to prepare for factoring out the "wait for a
peer" step. Namely, this means using an event signal we can wait on from
outside of this method.

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-5-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 1b9c8cb6ce6b5c5911eb715b2d5b0a2671999dde
      
https://github.com/qemu/qemu/commit/1b9c8cb6ce6b5c5911eb715b2d5b0a2671999dde
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py

  Log Message:
  -----------
  python/aqmp: squelch pylint warning for too many lines

I would really like to keep this under 1000 lines, I promise. Doesn't
look like it's gonna happen.

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-6-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 5e9902a030ab832b0b6577764c65ce6a6f874af6
      
https://github.com/qemu/qemu/commit/5e9902a030ab832b0b6577764c65ce6a6f874af6
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py
    M python/tests/protocol.py

  Log Message:
  -----------
  python/aqmp: refactor _do_accept() into two distinct steps

Refactor _do_accept() into _do_start_server() and _do_accept(). As of
this commit, the former calls the latter, but in subsequent commits
they'll be split apart.

(So please forgive the misnomer for _do_start_server(); it will live up
to its name shortly, and the docstring will be updated then too. I'm
just cutting down on some churn.)

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-7-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 32c5abf051d06ff103d9d30eb6a7f3e8bf582334
      
https://github.com/qemu/qemu/commit/32c5abf051d06ff103d9d30eb6a7f3e8bf582334
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py

  Log Message:
  -----------
  python/aqmp: stop the server during disconnect()

Before we allow the full separation of starting the server and accepting
new connections, make sure that the disconnect cleans up the server and
its new state, too.

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-8-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 481607c7d35de2bc4d9bec7f4734036fc467f330
      
https://github.com/qemu/qemu/commit/481607c7d35de2bc4d9bec7f4734036fc467f330
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/protocol.py
    M python/tests/protocol.py

  Log Message:
  -----------
  python/aqmp: add start_server() and accept() methods

Add start_server() and accept() methods that can be used instead of
start_server_and_accept() to allow more fine-grained control over the
incoming connection process.

(Eagle-eyed reviewers will surely notice that it's a bit weird that
"CONNECTING" is a state that's shared between both the start_server()
and connect() states. That's absolutely true, and it's very true that
checking on the presence of _accepted as an indicator of state is a
hack. That's also very certainly true. But ... this keeps client code an
awful lot simpler, as it doesn't have to care exactly *how* the
connection is being made, just that it *is*. Is it worth disrupting that
simplicity in order to provide a better state guard on `accept()`? Hm.)

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-9-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 673856f9d889dc50b6a1a7964df960c4f00c7c93
      
https://github.com/qemu/qemu/commit/673856f9d889dc50b6a1a7964df960c4f00c7c93
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/legacy.py

  Log Message:
  -----------
  python/aqmp: fix race condition in legacy.py

legacy.py provides a synchronous model. iotests frequently uses this
paradigm:

 - create QMP client object
 - start QEMU process
 - await connection from QEMU process

In the switch from sync to async QMP, the QMP client object stopped
calling bind() and listen() during the QMP object creation step, which
creates a race condition if the QEMU process dials in too quickly.

With refactoring out of the way, restore the former behavior of calling
bind() and listen() during __init__() to fix this race condition.

Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-10-jsnow@redhat.com
[Expanded commit message. --js]
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 4c1fe7003c9b373acb0791b4356e2285a10365c0
      
https://github.com/qemu/qemu/commit/4c1fe7003c9b373acb0791b4356e2285a10365c0
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/legacy.py
    M python/qemu/aqmp/protocol.py

  Log Message:
  -----------
  python/aqmp: drop _bind_hack()

_bind_hack() was a quick fix to allow async QMP to call bind(2) prior to
calling listen(2) and accept(2). This wasn't sufficient to fully address
the race condition present in synchronous clients.

With the race condition in legacy.py fixed (see the previous commit),
there are no longer any users of _bind_hack(). Drop it.

Fixes: b0b662bb2b3
Signed-off-by: John Snow <jsnow@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20220225205948.3693480-11-jsnow@redhat.com
[Expanded commit message. --js]
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 7cba010e821bf227e5fa016d0df06f2a33a0c318
      
https://github.com/qemu/qemu/commit/7cba010e821bf227e5fa016d0df06f2a33a0c318
  Author: John Snow <jsnow@redhat.com>
  Date:   2022-03-07 (Mon, 07 Mar 2022)

  Changed paths:
    M scripts/qmp/qmp-shell-wrap

  Log Message:
  -----------
  scripts/qmp-shell-wrap: Fix import path

Mea culpa. Dan's patch wound up with the wrong import path because I
re-ordered my most recent pull request and missed that this needed a fix
on rebase.

Fixes: 43912529
Reported-by: Kashyap Chamarthy <kchamart@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Tested-by: Kashyap Chamarthy <kchamart@redhat.com>
Message-id: 20220225170828.3418305-1-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>


  Commit: 2ad76249000dc35f0a588bd55bd9264f567b4abc
      
https://github.com/qemu/qemu/commit/2ad76249000dc35f0a588bd55bd9264f567b4abc
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2022-03-08 (Tue, 08 Mar 2022)

  Changed paths:
    M python/qemu/aqmp/legacy.py
    M python/qemu/aqmp/protocol.py
    M python/tests/protocol.py
    M scripts/qmp/qmp-shell-wrap

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/jsnow-gitlab/tags/python-pull-request' 
into staging

Python patches

Hopefully, fixes the race conditions witnessed through the NetBSD vm tests.

# gpg: Signature made Mon 07 Mar 2022 22:14:42 GMT
# gpg:                using RSA key F9B7ABDBBCACDF95BE76CBD07DEF8106AAFC390E
# gpg: Good signature from "John Snow (John Huston) <jsnow@redhat.com>" [full]
# Primary key fingerprint: FAEB 9711 A12C F475 812F  18F2 88A9 064D 1835 61EB
#      Subkey fingerprint: F9B7 ABDB BCAC DF95 BE76  CBD0 7DEF 8106 AAFC 390E

* remotes/jsnow-gitlab/tags/python-pull-request:
  scripts/qmp-shell-wrap: Fix import path
  python/aqmp: drop _bind_hack()
  python/aqmp: fix race condition in legacy.py
  python/aqmp: add start_server() and accept() methods
  python/aqmp: stop the server during disconnect()
  python/aqmp: refactor _do_accept() into two distinct steps
  python/aqmp: squelch pylint warning for too many lines
  python/aqmp: split _client_connected_cb() out as _incoming()
  python/aqmp: remove _new_session and _establish_connection
  python/aqmp: rename 'accept()' to 'start_server_and_accept()'
  python/aqmp: add _session_guard()

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Compare: https://github.com/qemu/qemu/compare/9a61e6c7e121...2ad76249000d