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

[John Snow]

On Thu, Feb 3, 2022 at 4:38 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Feb 02, 2022 at 02:08:59PM -0500, John Snow wrote:
> > On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > 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+.)
> > > 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?
> > >
> >
> > Hm, honestly, no. I think you'll still need the two-phase in the sync
> > version no matter what -- if you expect to be able to launch QMP and
> > QEMU from the same process, anyway. I suppose it's just a matter of
> > what you *call* it and what/where the arguments are.
> >
> > I could just call it bind(), and it does whatever it needs to (Either
> > creating a socket or, in 3.7+, instantiating more of the asyncio loop
> > machinery).
> > The signature for accept() would need to change to (perhaps)
> > optionally accepting no arguments; i.e. you can do either of the
> > following:
> >
> > (1) qmp.bind('/tmp/sock')
> >     qmp.accept()
> >
> > (2) qmp.accept('/tmp/sock')
> >
> > The risk is that the interface is just a touch more complex, the docs
> > get a bit more cluttered, there's a slight loss of clarity, the
> > accept() method would need to check to make sure you didn't give it an
> > address argument if you've already given it one, etc. Necessary
> > trade-off for the flexibility, though.
>
> Doing a bind() inside an accept() call is really strange
> design IMHO.
>

Yeah, it was possibly a mistake to use the same names as the very
famous and widely known POSIX calls. I didn't intend for them to be
precisely 1:1.

What I intended was that each QMPClient() object could either receive
an incoming connection or dial out and make one. I happened to name
these accept() and connect(). Under the hood, we do "whatever we have
to" in order to make those methods work. This was the simplest
possible interface I could imagine; and it also mimics the (sync) qmp
code in python/qemu/qmp. That code doesn't have distinct
bind-listen-accept steps; it performs bind and listen immediately
during __init__, then socket.accept() is called during the accept()
method call.

However, In python asyncio, we're encouraged to use the
asyncio.start_server() and asyncio.start_unix_server() functions
instead. These are async calls, and they cannot occur during
__init__(), because __init__() isn't an async coroutine. This compels
us to move the bind step out of init and into an async method. In the
interest of keeping the new interface looking very similar to the old
one, I pushed the entirety of the bind-listen-accept sequence into the
async accept(...) method.

A little weird, but that's why it happened that way. (The concept of a
QMP client being the socket server is also a little weird and defies
the traditional expectation, too, IMO. Oh well, we're here now.)

For comparison, the sync interface:

qmp = QEMUMonitorProtocol('qemu.sock', server=True)  # bind(), listen()
qmp.accept()  # accept()

And the async interface:

qmp = QMPClient()
await qmp.accept('qemu.sock')  # bind, listen, and accept

That said, I could probably make this interface a lot more traditional
for actual /servers/. I have patches to add a QMPServer() class, but
it adopted the "exactly one peer" limitation that QMPClient() has. If
you think there is some value in developing the idea further, I could
refactor the code to be a *lot* more traditional and have a listening
server that acts as a factory for generating connected client
instances. It just isn't something that I think I'll have merge ready
in less than a week.

> bind() is a one-time-only initialization operation for startup,
> where as accept() is a runtime operation invoked repeatedly.
>
> bind() is also an op that is reasonably likely to fail
> due to something already holding the socket address, and thus
> an error condition that you want to report to an application
> during its early startup phase.

Yeah, oops. I was focused on keeping the number of public calls low,
but that didn't help me out here.

Lemme see what I can do.

--js