Re: [PATCH v4 4/4] python/aqmp: add socket bind step to legacy.py

[Kevin Wolf]

Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > The synchronous QMP library would bind to the server address during
> > > __init__(). The new library delays this to the accept() call, because
> > > binding occurs inside of the call to start_[unix_]server(), which is an
> > > async method -- so it cannot happen during __init__ anymore.
> > >
> > > Python 3.7+ adds the ability to create the server (and thus the bind()
> > > call) and begin the active listening in separate steps, but we don't
> > > have that functionality in 3.6, our current minimum.
> > >
> > > Therefore ... Add a temporary workaround that allows the synchronous
> > > version of the client to bind the socket in advance, guaranteeing that
> > > there will be a UNIX socket in the filesystem ready for the QEMU client
> > > to connect to without a race condition.
> > >
> > > (Yes, it's a bit ugly. Fixing it more nicely will have to wait until our
> > > minimum Python version is 3.7+.)
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >  python/qemu/aqmp/legacy.py   |  3 +++
> > >  python/qemu/aqmp/protocol.py | 41 +++++++++++++++++++++++++++++++++---
> > >  2 files changed, 41 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> > > index 0890f95b16..6baa5f3409 100644
> > > --- a/python/qemu/aqmp/legacy.py
> > > +++ b/python/qemu/aqmp/legacy.py
> > > @@ -56,6 +56,9 @@ def __init__(self, address: SocketAddrT,
> > >          self._address = address
> > >          self._timeout: Optional[float] = None
> > >
> > > +        if server:
> > > +            self._aqmp._bind_hack(address)  # pylint: 
> > > disable=protected-access
> >
> > I feel that this is the only part that really makes it ugly. Do you
> > really think this way is so bad that we can't make it an official public
> > interface in the library?
> >
> > Kevin
> >
> 
> Good question.
> 
> I felt like I'd rather use the 'start_serving' parameter of
> loop.create_server(...), added in python 3.7; see
> https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server
> Python 3.6 is already EOL, but we still depend on it for our build and
> I wasn't prepared to write the series that forces us on to 3.7,
> because RHEL uses 3.6 as its native python. I'll have to update the
> docker files, etc -- and I'm sure people will be kind of unhappy with
> this, so I am putting it off. People were unhappy enough with the move
> to Python 3.6.

Yes, I understand. In theory - I haven't really thought much about it -
it feels like whether you create a separate socket in a first step and
pass it to the server that is created in the second step (for 3.6) or
you start an idle server in the first step and then let it start serving
in the second step (for 3.7+) should be an implementation detail if we
get the external API right.

> I also felt as though the async version has no real need for a
> separate bind step -- you can just start the server in a coroutine and
> wait until it yields, then proceed to launch QEMU. It's easy in that
> paradigm. If this bind step is only for the benefit of the legacy.py
> interface, I thought maybe it wasn't wise to commit to supporting it
> if it was something I wanted to get rid of soon anyway. There's also
> the ugliness that if you use the early bind step, the arguments passed
> to accept() are now ignored, which is kind of ugly, too. It's not a
> *great* interface. It doesn't spark joy.

Right, that's probably a sign that the interfaces aren't completely
right. I'm not sure which interfaces. As long as it's just _bind_hack()
and it's only used in an API that is going away in the future, that's no
problem. But it could also be a sign that the public interfaces aren't
flexible enough.

> I have some patches that are building a "sync.py" module that's meant
> to replace "legacy.py", and it's in the development of that module
> that I expect to either remove the bits I am unhappy with, or commit
> to supporting necessary infrastructure that's just simply required for
> a functional synchronous interface to work. I planned to start
> versioning the "qemu.qmp" package at 0.0.1, and the version that drops
> legacy.py I intend to version at 0.1.0.

If I understand these version numbers correctly, this also implies that
there is no compatibility promise yet. So maybe what to make a public
interface isn't even a big concern yet.

I guess the relevant question in the context of this patch is whether
sync.py will need a similar two-phase setup as legacy.py. Do you think
you can do without it when you have to reintroduce this behaviour here
to fix bugs?

> All that said, I'm fairly wishy-washy on it, so if you have some
> strong feelings, lemme know. There may be some real utility in just
> doubling down on always creating our own socket object. I just haven't
> thought through everything here, admittedly.

No, I don't have strong feelings about this. I don't even really know
the code. I just saw that you were unhappy with the solution and thought
I could try to be the rubber duck that makes you figure out something
that you're happy with. ;-)

Kevin