Re: [Qemu-devel] [PATCH 6/9] iotests: Explicitly inherit FDs in Python

[Cleber Rosa <crosa@redhat.com>]

On 10/15/18 10:14 AM, Max Reitz wrote:
> Python 3.2 introduced the inheritable attribute for FDs.  At the same
> time, it changed the default so that all FDs are not inheritable by
> default, that only inheritable FDs are inherited to subprocesses, and
> only if close_fds is explicitly set to False.
> 

It's actually a change that was introduced in 3.4:

https://docs.python.org/3/library/os.html#inheritance-of-file-descriptors

> Adhere to this by setting close_fds to False when working with
> subprocesses that may want to inherit FDs, and by trying to
> set_inheritable() on FDs that we do want to bequeath to them.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  scripts/qemu.py        | 13 +++++++++++--
>  scripts/qmp/qmp.py     |  7 +++++++
>  tests/qemu-iotests/147 |  7 +++++++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index f099ce7278..28366c4a67 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -142,10 +142,18 @@ class QEMUMachine(object):
>          if opts:
>              options.append(opts)
>  
> +        # This did not exist before 3.2, but since then it is
> +        # mandatory for our purpose
> +        try:

Version should be 3.4 here as well.

> +            os.set_inheritable(fd, True)
> +        except AttributeError:
> +            pass
> +

Doing hasattr(os, "set_inheritable") is cheaper.

- Cleber.

>          self._args.append('-add-fd')
>          self._args.append(','.join(options))
>          return self
>  
> +    # The caller needs to make sure the FD is inheritable
>      def send_fd_scm(self, fd_file_path):
>          # In iotest.py, the qmp should always use unix socket.
>          assert self._qmp.is_scm_available()
> @@ -159,7 +167,7 @@ class QEMUMachine(object):
>                      "%s" % fd_file_path]
>          devnull = open(os.path.devnull, 'rb')
>          proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
> -                                stderr=subprocess.STDOUT)
> +                                stderr=subprocess.STDOUT, close_fds=False)
>          output = proc.communicate()[0]
>          if output:
>              LOG.debug(output)
> @@ -280,7 +288,8 @@ class QEMUMachine(object):
>                                         stdin=devnull,
>                                         stdout=self._qemu_log_file,
>                                         stderr=subprocess.STDOUT,
> -                                       shell=False)
> +                                       shell=False,
> +                                       close_fds=False)
>          self._post_launch()
>  
>      def wait(self):
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 5c8cf6a056..009be8345b 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -10,6 +10,7 @@
>  
>  import json
>  import errno
> +import os
>  import socket
>  import logging
>  
> @@ -253,4 +254,10 @@ class QEMUMonitorProtocol(object):
>          return self.__sock.fileno()
>  
>      def is_scm_available(self):
> +        # This did not exist before 3.2, but since then it is
> +        # mandatory for our purpose
> +        try:
> +            os.set_inheritable(self.get_sock_fd(), True)
> +        except AttributeError:
> +            pass
>          return self.__sock.family == socket.AF_UNIX
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> index d2081df84b..b58455645b 100755
> --- a/tests/qemu-iotests/147
> +++ b/tests/qemu-iotests/147
> @@ -229,6 +229,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>          sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
>          sockfd.connect(unix_socket)
>  
> +        # This did not exist before 3.2, but since then it is
> +        # mandatory for our purpose
> +        try:
> +            os.set_inheritable(sockfd.fileno(), True)
> +        except AttributeError:
> +            pass
> +
>          result = self.vm.send_fd_scm(str(sockfd.fileno()))
>          self.assertEqual(result, 0, 'Failed to send socket FD')
>  
>