Re: [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection

[John Snow]

On Tue, Feb 7, 2023 at 4:04 PM Steven Sistare <steven.sistare@oracle.com> wrote:
>
> On 2/7/2023 3:28 PM, John Snow wrote:
> > On Tue, Feb 7, 2023 at 2:03 PM Steve Sistare <steven.sistare@oracle.com> 
> > wrote:
> >>
> >> Provide reopen_qmp_connection() to reopen a closed monitor connection.
> >> This will be needed by cpr, because qemu exec closes the monitor socket.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> Reviewed-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  python/qemu/machine/machine.py | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/python/qemu/machine/machine.py 
> >> b/python/qemu/machine/machine.py
> >> index ef94dcf..557209a 100644
> >> --- a/python/qemu/machine/machine.py
> >> +++ b/python/qemu/machine/machine.py
> >> @@ -501,6 +501,16 @@ def _close_qmp_connection(self) -> None:
> >>          finally:
> >>              self._qmp_connection = None
> >>
> >> +    def reopen_qmp_connection(self) -> None:
> >> +        """Close and re-open the QMP connection."""
> >> +        self._close_qmp_connection()
> >> +        self._qmp_connection = QEMUMonitorProtocol(
> >> +            self._monitor_address,
> >> +            server=True,
> >> +            nickname=self._name
> >> +        )
> >> +        self._qmp.accept(self._qmp_timer)
> >> +
> >>      def _early_cleanup(self) -> None:
> >>          """
> >>          Perform any cleanup that needs to happen before the VM exits.
> >> --
> >> 1.8.3.1
> >>
> >
> > This code is still mechanically fine as far as I can tell, but I lost
> > the plot on why it's needed - Can you please elaborate for me on what
> > you mean by "qemu exec will close the socket"?
> >
> > (R-B still stands, but since I am rewriting machine.py to be natively
> > async, I should be aware of your new use case.)
>
> Sure, thanks for asking.
>
> During cpr (aka live update), the old qemu process directly execv's the new
> qemu binary.  The monitor socket fd is marked cloexec, so the kernel closes it
> during execv.
>
> - Steve

Oho, I see.

I wonder if you are helped at all by some of Marc-Andre's recent
changes to use socketpair() for QMP connections with machine.py.
Depending on how those sockets are managed, it might be possible to
hold onto one of them without having it closed alongside the old QEMU
process. Or, if that doesn't help, double-check that it doesn't *hurt*
you either. It should be the default as of about a week ago now --
instead of using a local unix socket, we use socketpair and pass the
FD to help alleviate some race conditions.

(Again, I'm fine with this patch either way, just a heads up.)

--js