Re: [PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[John Snow]

On Wed, Jan 18, 2023 at 3:03 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Jan 18, 2023 at 2:36 AM John Snow <jsnow@redhat.com> wrote:
> >
> > On Wed, Jan 11, 2023 at 3:01 AM <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > When no monitor address is given, establish the QMP communication through
> > > a socketpair() (API is also supported on Windows since Python 3.5)
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  python/qemu/machine/machine.py | 24 ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/python/qemu/machine/machine.py 
> > > b/python/qemu/machine/machine.py
> > > index 748a0d807c..5b2e499e68 100644
> > > --- a/python/qemu/machine/machine.py
> > > +++ b/python/qemu/machine/machine.py
> > > @@ -158,17 +158,13 @@ def __init__(self,
> > >          self._qmp_timer = qmp_timer
> > >
> > >          self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
> > > +        self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = 
> > > None
> > >          self._temp_dir: Optional[str] = None
> > >          self._base_temp_dir = base_temp_dir
> > >          self._sock_dir = sock_dir
> > >          self._log_dir = log_dir
> > >
> > > -        if monitor_address is not None:
> > > -            self._monitor_address = monitor_address
> > > -        else:
> > > -            self._monitor_address = os.path.join(
> > > -                self.sock_dir, f"{self._name}-monitor.sock"
> > > -            )
> > > +        self._monitor_address = monitor_address
> > >
> > >          self._console_log_path = console_log
> > >          if self._console_log_path:
> > > @@ -303,7 +299,11 @@ def _base_args(self) -> List[str]:
> > >          args = ['-display', 'none', '-vga', 'none']
> > >
> > >          if self._qmp_set:
> > > -            if isinstance(self._monitor_address, tuple):
> > > +            if self._sock_pair:
> > > +                fd = self._sock_pair[0].fileno()
> > > +                os.set_inheritable(fd, True)
> > > +                moncdev = f"socket,id=mon,fd={fd}"
> > > +            elif isinstance(self._monitor_address, tuple):
> > >                  moncdev = "socket,id=mon,host={},port={}".format(
> > >                      *self._monitor_address
> > >                  )
> > > @@ -337,10 +337,17 @@ def _pre_launch(self) -> None:
> > >              self._remove_files.append(self._console_address)
> > >
> > >          if self._qmp_set:
> > > +            monitor_address = None
> > > +            sock = None
> > > +            if self._monitor_address is None:
> > > +                self._sock_pair = socket.socketpair()
> > > +                sock = self._sock_pair[1]
> > >              if isinstance(self._monitor_address, str):
> > >                  self._remove_files.append(self._monitor_address)
> > > +                monitor_address = self._monitor_address
> > >              self._qmp_connection = QEMUMonitorProtocol(
> > > -                self._monitor_address,
> > > +                address=monitor_address,
> > > +                sock=sock,
> > >                  server=True,
> > >                  nickname=self._name
> > >              )
> > > @@ -360,6 +367,7 @@ def _pre_launch(self) -> None:
> > >          ))
> > >
> > >      def _post_launch(self) -> None:
> > > +        self._sock_pair[0].close()
> >
> > Needs an assert or an error-check here for _sock_pair to be non-None,
> > otherwise mypy will shout. Try running "make check-dev" from
> > qemu.git/python directory as a smoke test.
>
> Good catch, it should be:
>
> +        if self._sock_pair:
> +            self._sock_pair[0].close()
>
> Let me know if you want me to resend the whole series, or if you can
> touch it during picking.

Touching it up during picking, running tests, PR soon. Thanks,
--js