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

[Daniel P . Berrangé]

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.

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|