[PATCH 00/10] Python: Fix qmp race condition on accept()

John Snow posted 10 patches 2 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220225205948.3693480-1-jsnow@redhat.com
Maintainers: John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>, Beraldo Leal <bleal@redhat.com>
python/qemu/aqmp/legacy.py   |   7 +-
python/qemu/aqmp/protocol.py | 393 +++++++++++++++++++++--------------
python/tests/protocol.py     |  45 ++--
3 files changed, 273 insertions(+), 172 deletions(-)
[PATCH 00/10] Python: Fix qmp race condition on accept()
Posted by John Snow 2 years, 2 months ago
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-accept-changes
CI: https://gitlab.com/jsnow/qemu/-/pipelines/479795153

This redesigns the async QMP interface to allow for race-free
connections from the synchronous interface. It should hopefully address
the race conditions Peter has been seeing on the NetBSD vm tests.

John Snow (10):
  python/aqmp: add _session_guard()
  python/aqmp: rename 'accept()' to 'start_server_and_accept()'
  python/aqmp: remove _new_session and _establish_connection
  python/aqmp: split _client_connected_cb() out as _incoming()
  python/aqmp: squelch pylint warning for too many lines
  python/aqmp: refactor _do_accept() into two distinct steps
  python/aqmp: stop the server during disconnect()
  python/aqmp: add start_server() and accept() methods
  python/aqmp: fix race condition in legacy.py
  python/aqmp: drop _bind_hack()

 python/qemu/aqmp/legacy.py   |   7 +-
 python/qemu/aqmp/protocol.py | 393 +++++++++++++++++++++--------------
 python/tests/protocol.py     |  45 ++--
 3 files changed, 273 insertions(+), 172 deletions(-)

-- 
2.34.1



Re: [PATCH 00/10] Python: Fix qmp race condition on accept()
Posted by John Snow 2 years, 2 months ago
Ping - Dan, Kevin: Any thoughts on the new API here? I only ask
because there was a bit of feedback in response to the last patch and
I didn't want to stage this without giving you a fair chance to look.

I'm not expecting any real review on the Python, just wanted to see if
you felt like this design adequately addressed your feedback from
before.

Here is the super high level:

1. what was accept() is renamed start_server_and_accept(). It's a
one-shot command to do (effectively) bind(2) listen(2) accept(2).
2. start_server() method is added. It calls bind() and listen(), and
new connections will be accept(2)'d asynchronously.
3. accept() method is added. It does not *actually* call accept(2),
it's more like "wait for accept(2) to be called in the bottom half".
Similar enough.
4. start_server_and_accept() is, by the end of the series, literally
just two calls to start_server() and accept().

Otherwise, I'll just assume no news is good news and I'll send a
pullreq soon so that Peter's NetBSD tests stop being flaky.

--js


On Fri, Feb 25, 2022 at 4:00 PM John Snow <jsnow@redhat.com> wrote:
>
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-accept-changes
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/479795153
>
> This redesigns the async QMP interface to allow for race-free
> connections from the synchronous interface. It should hopefully address
> the race conditions Peter has been seeing on the NetBSD vm tests.
>
> John Snow (10):
>   python/aqmp: add _session_guard()
>   python/aqmp: rename 'accept()' to 'start_server_and_accept()'
>   python/aqmp: remove _new_session and _establish_connection
>   python/aqmp: split _client_connected_cb() out as _incoming()
>   python/aqmp: squelch pylint warning for too many lines
>   python/aqmp: refactor _do_accept() into two distinct steps
>   python/aqmp: stop the server during disconnect()
>   python/aqmp: add start_server() and accept() methods
>   python/aqmp: fix race condition in legacy.py
>   python/aqmp: drop _bind_hack()
>
>  python/qemu/aqmp/legacy.py   |   7 +-
>  python/qemu/aqmp/protocol.py | 393 +++++++++++++++++++++--------------
>  python/tests/protocol.py     |  45 ++--
>  3 files changed, 273 insertions(+), 172 deletions(-)
>
> --
> 2.34.1
>
>
Re: [PATCH 00/10] Python: Fix qmp race condition on accept()
Posted by Kevin Wolf 2 years, 2 months ago
Am 25.02.2022 um 21:59 hat John Snow geschrieben:
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-accept-changes
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/479795153
> 
> This redesigns the async QMP interface to allow for race-free
> connections from the synchronous interface. It should hopefully address
> the race conditions Peter has been seeing on the NetBSD vm tests.

Acked-by: Kevin Wolf <kwolf@redhat.com>
Re: [PATCH 00/10] Python: Fix qmp race condition on accept()
Posted by John Snow 2 years, 2 months ago
On Fri, Mar 4, 2022 at 12:49 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 25.02.2022 um 21:59 hat John Snow geschrieben:
> > GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-accept-changes
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/479795153
> >
> > This redesigns the async QMP interface to allow for race-free
> > connections from the synchronous interface. It should hopefully address
> > the race conditions Peter has been seeing on the NetBSD vm tests.
>
> Acked-by: Kevin Wolf <kwolf@redhat.com>
>

Thanks so much to the both of you!

I was dreading this fix, but it wound up being *pretty* clean in the
end, I think. I really appreciate the double-check here.

--js