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

[John Snow]

On Thu, Feb 3, 2022 at 4:19 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 02.02.2022 um 20:08 hat John Snow geschrieben:
> > > 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.
>
> Hm, how about copying the create_server() interface instead? So it
> would become:
>
> (1) qmp.create_server('/tmp/sock', start_serving=False)
>     qmp.start_serving()
>
> (2) qmp.create_server('/tmp/sock')
>
> Then you get the connection details only in a single place. (The names
> should probably be changed because we're still a QMP client even though
> we're technically the server on the socket level, but you get the idea.)

Good idea. I'll experiment.

(I'm worried that the way I initially factored the code to begin with
might make this ugly, but maybe I'm just dreading nothing. I'll have
to see.)

Thanks for taking a look!

--js