[PATCH 0/7] python: More fixes for 6.2

[PATCH 0/7] python: More fixes for 6.2

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/pipelines/412040104

Patch 5 fixes a race condition in machine.py exposed by device-crash-test.
Patch 6 fixes a hang in device-crash-test.
Patch 7 fixes send_fd_scm in AQMP under Python 3.6.

Patches 1-3 provide a minor concurrency fix that happened to get fixed
on my way to fixing the above three.
Patch 4 is just a trivial style thing.

With this, device-crash-test should be back to producing only strictly
relevant output.

John Snow (7):
  python/machine: add @sock_dir property
  python/machine: remove _remove_monitor_sockfile property
  python/machine: add instance disambiguator to default nickname
  python/machine: move more variable initializations to _pre_launch
  python/machine: handle "fast" QEMU terminations
  scripts/device-crash-test: Use a QMP timeout
  python/aqmp: fix send_fd_scm for python 3.6.x

 python/qemu/aqmp/qmp_client.py |  9 ++++--
 python/qemu/machine/machine.py | 59 ++++++++++++++++++++--------------
 scripts/device-crash-test      |  2 +-
 3 files changed, 42 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH 1/7] python/machine: add @sock_dir property

[John Snow]

Analogous to temp_dir and log_dir, add a sock_dir property that defaults
to @temp_dir -- instead of base_temp_dir -- when the user hasn't
overridden the sock dir value in the initializer.

This gives us a much more unique directory to put sockfiles in by default.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a487c39745..b1dd77b538 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -134,8 +134,9 @@ def __init__(self,
         self._qmp_timer = qmp_timer
 
         self._name = name or "qemu-%d" % os.getpid()
+        self._temp_dir: Optional[str] = None
         self._base_temp_dir = base_temp_dir
-        self._sock_dir = sock_dir or self._base_temp_dir
+        self._sock_dir = sock_dir
         self._log_dir = log_dir
 
         if monitor_address is not None:
@@ -143,7 +144,7 @@ def __init__(self,
             self._remove_monitor_sockfile = False
         else:
             self._monitor_address = os.path.join(
-                self._sock_dir, f"{self._name}-monitor.sock"
+                self.sock_dir, f"{self._name}-monitor.sock"
             )
             self._remove_monitor_sockfile = True
 
@@ -163,14 +164,13 @@ def __init__(self,
         self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp_connection: Optional[QEMUMonitorProtocol] = None
         self._qemu_full_args: Tuple[str, ...] = ()
-        self._temp_dir: Optional[str] = None
         self._launched = False
         self._machine: Optional[str] = None
         self._console_index = 0
         self._console_set = False
         self._console_device_type: Optional[str] = None
         self._console_address = os.path.join(
-            self._sock_dir, f"{self._name}-console.sock"
+            self.sock_dir, f"{self._name}-console.sock"
         )
         self._console_socket: Optional[socket.socket] = None
         self._remove_files: List[str] = []
@@ -816,6 +816,15 @@ def temp_dir(self) -> str:
                                               dir=self._base_temp_dir)
         return self._temp_dir
 
+    @property
+    def sock_dir(self) -> str:
+        """
+        Returns the directory used for sockfiles by this machine.
+        """
+        if self._sock_dir:
+            return self._sock_dir
+        return self.temp_dir
+
     @property
     def log_dir(self) -> str:
         """
-- 
2.31.1

Re: [PATCH 1/7] python/machine: add @sock_dir property

[Willian Rampazzo]

On Thu, Nov 18, 2021 at 5:50 PM John Snow <jsnow@redhat.com> wrote:
>
> Analogous to temp_dir and log_dir, add a sock_dir property that defaults
> to @temp_dir -- instead of base_temp_dir -- when the user hasn't
> overridden the sock dir value in the initializer.
>
> This gives us a much more unique directory to put sockfiles in by default.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

[PATCH 2/7] python/machine: remove _remove_monitor_sockfile property

[John Snow]

It doesn't matter if it was the user or the class itself that specified
where the sockfile should be created; the fact is that if we are using a
sockfile here, we created it and we can clean it up.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index b1dd77b538..ea9e07805d 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -141,12 +141,10 @@ def __init__(self,
 
         if monitor_address is not None:
             self._monitor_address = monitor_address
-            self._remove_monitor_sockfile = False
         else:
             self._monitor_address = os.path.join(
                 self.sock_dir, f"{self._name}-monitor.sock"
             )
-            self._remove_monitor_sockfile = True
 
         self._console_log_path = console_log
         if self._console_log_path:
@@ -315,8 +313,7 @@ def _pre_launch(self) -> None:
             self._remove_files.append(self._console_address)
 
         if self._qmp_set:
-            if self._remove_monitor_sockfile:
-                assert isinstance(self._monitor_address, str)
+            if isinstance(self._monitor_address, str):
                 self._remove_files.append(self._monitor_address)
             self._qmp_connection = QEMUMonitorProtocol(
                 self._monitor_address,
-- 
2.31.1

Re: [PATCH 2/7] python/machine: remove _remove_monitor_sockfile property

[Willian Rampazzo]

On Thu, Nov 18, 2021 at 5:49 PM John Snow <jsnow@redhat.com> wrote:
>
> It doesn't matter if it was the user or the class itself that specified
> where the sockfile should be created; the fact is that if we are using a
> sockfile here, we created it and we can clean it up.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

[PATCH 3/7] python/machine: add instance disambiguator to default nickname

[John Snow]

If you create two instances of QEMUMachine(), they'll both create the
same nickname by default -- which is not that helpful.

Luckily, they'll both create unique temporary directories ... but due to
user configuration, they may share logging and sockfile directories,
meaning two instances can collide. The Python logging will also be quite
confusing, with no differentiation between the two instances.

Add an instance disambiguator (The memory address of the instance) to
the default nickname to foolproof this in all cases.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ea9e07805d..ad529fd92a 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -133,7 +133,7 @@ def __init__(self,
         self._wrapper = wrapper
         self._qmp_timer = qmp_timer
 
-        self._name = name or "qemu-%d" % os.getpid()
+        self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
         self._temp_dir: Optional[str] = None
         self._base_temp_dir = base_temp_dir
         self._sock_dir = sock_dir
-- 
2.31.1

Re: [PATCH 3/7] python/machine: add instance disambiguator to default nickname

[Philippe Mathieu-Daudé]

On 11/18/21 21:46, John Snow wrote:
> If you create two instances of QEMUMachine(), they'll both create the
> same nickname by default -- which is not that helpful.
> 
> Luckily, they'll both create unique temporary directories ... but due to
> user configuration, they may share logging and sockfile directories,
> meaning two instances can collide. The Python logging will also be quite
> confusing, with no differentiation between the two instances.
> 
> Add an instance disambiguator (The memory address of the instance) to
> the default nickname to foolproof this in all cases.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 3/7] python/machine: add instance disambiguator to default nickname

[Willian Rampazzo]

On Thu, Nov 18, 2021 at 5:49 PM John Snow <jsnow@redhat.com> wrote:
>
> If you create two instances of QEMUMachine(), they'll both create the
> same nickname by default -- which is not that helpful.
>
> Luckily, they'll both create unique temporary directories ... but due to
> user configuration, they may share logging and sockfile directories,
> meaning two instances can collide. The Python logging will also be quite
> confusing, with no differentiation between the two instances.
>
> Add an instance disambiguator (The memory address of the instance) to
> the default nickname to foolproof this in all cases.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

[PATCH 4/7] python/machine: move more variable initializations to _pre_launch

[John Snow]

No need to clear them only to set them later.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ad529fd92a..f92e73de40 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -327,6 +327,14 @@ def _pre_launch(self) -> None:
         self._qemu_log_path = os.path.join(self.log_dir, self._name + ".log")
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
+        self._iolog = None
+        self._qemu_full_args = tuple(chain(
+            self._wrapper,
+            [self._binary],
+            self._base_args,
+            self._args
+        ))
+
     def _post_launch(self) -> None:
         if self._qmp_connection:
             self._qmp.accept(self._qmp_timer)
@@ -390,8 +398,6 @@ def launch(self) -> None:
         if self._launched:
             raise QEMUMachineError('VM already launched')
 
-        self._iolog = None
-        self._qemu_full_args = ()
         try:
             self._launch()
             self._launched = True
@@ -410,12 +416,6 @@ def _launch(self) -> None:
         Launch the VM and establish a QMP connection
         """
         self._pre_launch()
-        self._qemu_full_args = tuple(
-            chain(self._wrapper,
-                  [self._binary],
-                  self._base_args,
-                  self._args)
-        )
         LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
 
         # Cleaning up of this subprocess is guaranteed by _do_shutdown.
-- 
2.31.1

Re: [PATCH 4/7] python/machine: move more variable initializations to _pre_launch

[Willian Rampazzo]

On Thu, Nov 18, 2021 at 5:51 PM John Snow <jsnow@redhat.com> wrote:
>
> No need to clear them only to set them later.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

[PATCH 5/7] python/machine: handle "fast" QEMU terminations

[John Snow]

In the case that the QEMU process actually launches -- but then dies so
quickly that we can't establish a QMP connection to it -- QEMUMachine
currently calls _post_shutdown() assuming that it never launched the VM
process.

This isn't true, though: it "merely" may have failed to establish a QMP
connection and the process is in the middle of its own exit path.

If we don't wait for the subprocess, the caller may get a bogus `None`
return for .exitcode(). This behavior was observed from
device-crash-test; after the switch to Async QMP, the timings were
changed such that it was now seemingly possible to witness the failure
of "vm.launch()" *prior* to the exitcode becoming available.

The semantic of the `_launched` property is changed in this
patch. Instead of representing the condition "launch() executed
successfully", it will now represent "has forked a child process
successfully". This way, wait() when called in the exit path won't
become a no-op.
Signed-off-by: John Snow <jsnow@redhat.com>

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index f92e73de40..67ab06ca2b 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -349,9 +349,6 @@ def _post_shutdown(self) -> None:
         Called to cleanup the VM instance after the process has exited.
         May also be called after a failed launch.
         """
-        # Comprehensive reset for the failed launch case:
-        self._early_cleanup()
-
         try:
             self._close_qmp_connection()
         except Exception as err:  # pylint: disable=broad-except
@@ -400,9 +397,16 @@ def launch(self) -> None:
 
         try:
             self._launch()
-            self._launched = True
         except:
-            self._post_shutdown()
+            # We may have launched the process but it may
+            # have exited before we could connect via QMP.
+            # Assume the VM didn't launch or is exiting.
+            # If we don't wait for the process, exitcode() may still be
+            # 'None' by the time control is ceded back to the caller.
+            if self._launched:
+                self.wait()
+            else:
+                self._post_shutdown()
 
             LOG.debug('Error launching VM')
             if self._qemu_full_args:
@@ -426,6 +430,7 @@ def _launch(self) -> None:
                                        stderr=subprocess.STDOUT,
                                        shell=False,
                                        close_fds=False)
+        self._launched = True
         self._post_launch()
 
     def _close_qmp_connection(self) -> None:
@@ -457,8 +462,8 @@ def _early_cleanup(self) -> None:
         """
         Perform any cleanup that needs to happen before the VM exits.
 
-        May be invoked by both soft and hard shutdown in failover scenarios.
-        Called additionally by _post_shutdown for comprehensive cleanup.
+        This method may be called twice upon shutdown, once each by soft
+        and hard shutdown in failover scenarios.
         """
         # If we keep the console socket open, we may deadlock waiting
         # for QEMU to exit, while QEMU is waiting for the socket to
-- 
2.31.1

Re: [PATCH 5/7] python/machine: handle "fast" QEMU terminations

[Willian Rampazzo]

On Thu, Nov 18, 2021 at 5:49 PM John Snow <jsnow@redhat.com> wrote:
>
> In the case that the QEMU process actually launches -- but then dies so
> quickly that we can't establish a QMP connection to it -- QEMUMachine
> currently calls _post_shutdown() assuming that it never launched the VM
> process.
>
> This isn't true, though: it "merely" may have failed to establish a QMP
> connection and the process is in the middle of its own exit path.
>
> If we don't wait for the subprocess, the caller may get a bogus `None`
> return for .exitcode(). This behavior was observed from
> device-crash-test; after the switch to Async QMP, the timings were
> changed such that it was now seemingly possible to witness the failure
> of "vm.launch()" *prior* to the exitcode becoming available.
>
> The semantic of the `_launched` property is changed in this
> patch. Instead of representing the condition "launch() executed
> successfully", it will now represent "has forked a child process
> successfully". This way, wait() when called in the exit path won't
> become a no-op.
> Signed-off-by: John Snow <jsnow@redhat.com>

Nitpick: double signed-off-by

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

[PATCH 6/7] scripts/device-crash-test: Use a QMP timeout

[John Snow]

Despite all the previous fixes, it's still possible for
device-crash-test to wedge itself in the case that QEMU terminates *so
quickly* that it doesn't even begin a connection attempt to our QMP
client. Python will just joyfully wait ad infinitum for a connection
that will now never arrive.

The real fix is to use asyncio to simultaneously poll both the health of
the launched process AND the connection attempt. That's quite a bit more
invasive than just setting a connection timeout, though.

Do the very simplest thing for now.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/device-crash-test | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 1c73dac93e..7fbd99158b 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -353,7 +353,7 @@ def checkOneCase(args, testcase):
             '-device', qemuOptsEscape(device)]
     cmdline = ' '.join([binary] + args)
     dbg("will launch QEMU: %s", cmdline)
-    vm = QEMUMachine(binary=binary, args=args)
+    vm = QEMUMachine(binary=binary, args=args, qmp_timer=15)
 
     exc = None
     exc_traceback = None
-- 
2.31.1

[PATCH 7/7] python/aqmp: fix send_fd_scm for python 3.6.x

[John Snow]

3.6 doesn't play keepaway with the socket object, so we don't need to go
fishing for it on this version. In fact, so long as 'sendmsg' is still
available, it's probably preferable to just use that method and only go
fishing for forbidden details when we absolutely have to.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/qmp_client.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index f987da02eb..8105e29fa8 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -639,9 +639,12 @@ def send_fd_scm(self, fd: int) -> None:
         if sock.family != socket.AF_UNIX:
             raise AQMPError("Sending file descriptors requires a UNIX socket.")
 
-        # Void the warranty sticker.
-        # Access to sendmsg in asyncio is scheduled for removal in Python 3.11.
-        sock = sock._sock  # pylint: disable=protected-access
+        if not hasattr(sock, 'sendmsg'):
+            # We need to void the warranty sticker.
+            # Access to sendmsg is scheduled for removal in Python 3.11.
+            # Find the real backing socket to use it anyway.
+            sock = sock._sock  # pylint: disable=protected-access
+
         sock.sendmsg(
             [b' '],
             [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
-- 
2.31.1

Re: [PATCH 7/7] python/aqmp: fix send_fd_scm for python 3.6.x

[Willian Rampazzo]

On Thu, Nov 18, 2021 at 5:53 PM John Snow <jsnow@redhat.com> wrote:
>
> 3.6 doesn't play keepaway with the socket object, so we don't need to go
> fishing for it on this version. In fact, so long as 'sendmsg' is still
> available, it's probably preferable to just use that method and only go
> fishing for forbidden details when we absolutely have to.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/qmp_client.py | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>