diff mbox series

[05/15] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol

Message ID 20210917054047.2042843-6-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series Switch iotests to using Async QMP | expand

Commit Message

John Snow Sept. 17, 2021, 5:40 a.m. UTC
It turns out you can do this directly from Python ... and because of
this, you don't need to worry about setting the inheritability of the
fds or spawning another process.

Doing this is helpful because it allows QEMUMonitorProtocol to keep its
file descriptor and socket object as private implementation details.

*that* is helpful in turn because it allows me to write a compatible,
 alternative implementation.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 44 +++++++---------------------------
 python/qemu/qmp/__init__.py    | 21 +++++++---------
 2 files changed, 18 insertions(+), 47 deletions(-)

Comments

Hanna Reitz Sept. 17, 2021, 1:21 p.m. UTC | #1
On 17.09.21 07:40, John Snow wrote:
> It turns out you can do this directly from Python ... and because of
> this, you don't need to worry about setting the inheritability of the
> fds or spawning another process.
>
> Doing this is helpful because it allows QEMUMonitorProtocol to keep its
> file descriptor and socket object as private implementation details.
>
> *that* is helpful in turn because it allows me to write a compatible,
>   alternative implementation.

Bit of a weird indentation here.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/machine/machine.py | 44 +++++++---------------------------
>   python/qemu/qmp/__init__.py    | 21 +++++++---------
>   2 files changed, 18 insertions(+), 47 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index ae945ca3c9..1c6532a3d6 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -213,48 +213,22 @@ def add_fd(self: _T, fd: int, fdset: int,
>       def send_fd_scm(self, fd: Optional[int] = None,
>                       file_path: Optional[str] = None) -> int:

[...]

>           if file_path is not None:
>               assert fd is None
> -            fd_param.append(file_path)
> +            with open(file_path, "rb") as passfile:
> +                fd = passfile.fileno()
> +                self._qmp.send_fd_scm(fd)

Seems a bit strange to send an fd that is then immediately closed, but 
that’s what socket_scm_helper did, and so it looks like the fd is 
effectively duplicated.  OK then.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
John Snow Sept. 17, 2021, 5:36 p.m. UTC | #2
On Fri, Sep 17, 2021 at 9:21 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 17.09.21 07:40, John Snow wrote:
> > It turns out you can do this directly from Python ... and because of
> > this, you don't need to worry about setting the inheritability of the
> > fds or spawning another process.
> >
> > Doing this is helpful because it allows QEMUMonitorProtocol to keep its
> > file descriptor and socket object as private implementation details.
> >
> > *that* is helpful in turn because it allows me to write a compatible,
> >   alternative implementation.
>
> Bit of a weird indentation here.
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/machine/machine.py | 44 +++++++---------------------------
> >   python/qemu/qmp/__init__.py    | 21 +++++++---------
> >   2 files changed, 18 insertions(+), 47 deletions(-)
> >
> > diff --git a/python/qemu/machine/machine.py
> b/python/qemu/machine/machine.py
> > index ae945ca3c9..1c6532a3d6 100644
> > --- a/python/qemu/machine/machine.py
> > +++ b/python/qemu/machine/machine.py
> > @@ -213,48 +213,22 @@ def add_fd(self: _T, fd: int, fdset: int,
> >       def send_fd_scm(self, fd: Optional[int] = None,
> >                       file_path: Optional[str] = None) -> int:
>
> [...]
>
> >           if file_path is not None:
> >               assert fd is None
> > -            fd_param.append(file_path)
> > +            with open(file_path, "rb") as passfile:
> > +                fd = passfile.fileno()
> > +                self._qmp.send_fd_scm(fd)
>
> Seems a bit strange to send an fd that is then immediately closed, but
> that’s what socket_scm_helper did, and so it looks like the fd is
> effectively duplicated.  OK then.
>
>
Same boat. It's weird, but it seems to work, and it's how the old interface
(ultimately) behaved, so ... https://i.imgur.com/O0CQXoh.png


> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
diff mbox series

Patch

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ae945ca3c9..1c6532a3d6 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -213,48 +213,22 @@  def add_fd(self: _T, fd: int, fdset: int,
     def send_fd_scm(self, fd: Optional[int] = None,
                     file_path: Optional[str] = None) -> int:
         """
-        Send an fd or file_path to socket_scm_helper.
+        Send an fd or file_path to the remote via SCM_RIGHTS.
 
-        Exactly one of fd and file_path must be given.
-        If it is file_path, the helper will open that file and pass its own fd.
+        Exactly one of fd and file_path must be given.  If it is
+        file_path, the file will be opened read-only and the new file
+        descriptor will be sent to the remote.
         """
-        # In iotest.py, the qmp should always use unix socket.
-        assert self._qmp.is_scm_available()
-        if self._socket_scm_helper is None:
-            raise QEMUMachineError("No path to socket_scm_helper set")
-        if not os.path.exists(self._socket_scm_helper):
-            raise QEMUMachineError("%s does not exist" %
-                                   self._socket_scm_helper)
-
-        # This did not exist before 3.4, but since then it is
-        # mandatory for our purpose
-        if hasattr(os, 'set_inheritable'):
-            os.set_inheritable(self._qmp.get_sock_fd(), True)
-            if fd is not None:
-                os.set_inheritable(fd, True)
-
-        fd_param = ["%s" % self._socket_scm_helper,
-                    "%d" % self._qmp.get_sock_fd()]
-
         if file_path is not None:
             assert fd is None
-            fd_param.append(file_path)
+            with open(file_path, "rb") as passfile:
+                fd = passfile.fileno()
+                self._qmp.send_fd_scm(fd)
         else:
             assert fd is not None
-            fd_param.append(str(fd))
+            self._qmp.send_fd_scm(fd)
 
-        proc = subprocess.run(
-            fd_param,
-            stdin=subprocess.DEVNULL,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
-            check=False,
-            close_fds=False,
-        )
-        if proc.stdout:
-            LOG.debug(proc.stdout)
-
-        return proc.returncode
+        return 0
 
     @staticmethod
     def _remove_if_exists(path: str) -> None:
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index ba15668c25..c3b8a70ec9 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -21,6 +21,7 @@ 
 import json
 import logging
 import socket
+import struct
 from types import TracebackType
 from typing import (
     Any,
@@ -408,18 +409,14 @@  def settimeout(self, timeout: Optional[float]) -> None:
             raise ValueError(msg)
         self.__sock.settimeout(timeout)
 
-    def get_sock_fd(self) -> int:
+    def send_fd_scm(self, fd: int) -> None:
         """
-        Get the socket file descriptor.
-
-        @return The file descriptor number.
-        """
-        return self.__sock.fileno()
-
-    def is_scm_available(self) -> bool:
+        Send a file descriptor to the remote via SCM_RIGHTS.
         """
-        Check if the socket allows for SCM_RIGHTS.
+        if self.__sock.family != socket.AF_UNIX:
+            raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
 
-        @return True if SCM_RIGHTS is available, otherwise False.
-        """
-        return self.__sock.family == socket.AF_UNIX
+        self.__sock.sendmsg(
+            [b' '],
+            [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+        )