* [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection @ 2023-02-07 19:03 Steve Sistare 2023-02-07 20:28 ` John Snow 0 siblings, 1 reply; 5+ messages in thread From: Steve Sistare @ 2023-02-07 19:03 UTC (permalink / raw) To: qemu-devel; +Cc: John Snow, Steve Sistare 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection 2023-02-07 19:03 [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection Steve Sistare @ 2023-02-07 20:28 ` John Snow 2023-02-07 21:04 ` Steven Sistare 0 siblings, 1 reply; 5+ messages in thread From: John Snow @ 2023-02-07 20:28 UTC (permalink / raw) To: Steve Sistare; +Cc: qemu-devel 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.) --js ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection 2023-02-07 20:28 ` John Snow @ 2023-02-07 21:04 ` Steven Sistare 2023-02-07 21:23 ` John Snow 0 siblings, 1 reply; 5+ messages in thread From: Steven Sistare @ 2023-02-07 21:04 UTC (permalink / raw) To: John Snow; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection 2023-02-07 21:04 ` Steven Sistare @ 2023-02-07 21:23 ` John Snow 2023-02-07 21:32 ` Steven Sistare 0 siblings, 1 reply; 5+ messages in thread From: John Snow @ 2023-02-07 21:23 UTC (permalink / raw) To: Steven Sistare; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection 2023-02-07 21:23 ` John Snow @ 2023-02-07 21:32 ` Steven Sistare 0 siblings, 0 replies; 5+ messages in thread From: Steven Sistare @ 2023-02-07 21:32 UTC (permalink / raw) To: John Snow; +Cc: qemu-devel On 2/7/2023 4:23 PM, John Snow wrote: > 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.) Thanks. Preserving the monitor connection and state across exec requires more work though, because of buffering, partially processed messages, and other state. I tried and abandoned that a while ago. Re-opening has been robust. - Steve ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-07 21:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-07 19:03 [PATCH V2] python/machine: QEMUMachine reopen_qmp_connection Steve Sistare 2023-02-07 20:28 ` John Snow 2023-02-07 21:04 ` Steven Sistare 2023-02-07 21:23 ` John Snow 2023-02-07 21:32 ` Steven Sistare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).