diff mbox series

[6/9] iotests: Explicitly inherit FDs in Python

Message ID 20181015141453.32632-7-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series iotests: Make them work for both Python 2 and 3 | expand

Commit Message

Max Reitz Oct. 15, 2018, 2:14 p.m. UTC
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.

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(-)

Comments

Eduardo Habkost Oct. 15, 2018, 8:30 p.m. UTC | #1
On Mon, Oct 15, 2018 at 04:14:50PM +0200, 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.
> 
> 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:
> +            os.set_inheritable(fd, True)
> +        except AttributeError:
> +            pass
> +

This is add_fd(), so calling set_inheritable() automatically here
makes sense.

>          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

Why did you decide to place this code inside is_scm_available()?

For reference, this is the only caller of is_scm_available():

    def send_fd_scm(self, fd_file_path):
        # In iotest.py, the qmp should always use unix socket.
        assert self._qmp.is_scm_available()
        ...

In addition to making a method called is_*() have an unexpected
side-effect, the method won't be called at all if running with
debugging disabled.

I suggest simply placing the os.set_inheritable() call inside
send_fd_scm(), as close as possible to the subprocess.Popen()
call.


>          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
> +

Why not make send_fd_scm() responsible for calling
os.set_inheritable(), making this hunk unnecessary?


>          result = self.vm.send_fd_scm(str(sockfd.fileno()))
>          self.assertEqual(result, 0, 'Failed to send socket FD')
>  
> -- 
> 2.17.1
>
Cleber Rosa Oct. 15, 2018, 11:18 p.m. UTC | #2
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')
>  
>
Max Reitz Oct. 19, 2018, 9:03 a.m. UTC | #3
On 15.10.18 22:30, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:50PM +0200, 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.
>>
>> 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:
>> +            os.set_inheritable(fd, True)
>> +        except AttributeError:
>> +            pass
>> +
> 
> This is add_fd(), so calling set_inheritable() automatically here
> makes sense.
> 
>>          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
> 
> Why did you decide to place this code inside is_scm_available()?
> 
> For reference, this is the only caller of is_scm_available():
> 
>     def send_fd_scm(self, fd_file_path):
>         # In iotest.py, the qmp should always use unix socket.
>         assert self._qmp.is_scm_available()
>         ...
> 
> In addition to making a method called is_*() have an unexpected
> side-effect,

True.  My idea was that a function that asks for SCM to be available
might as well make it available.

>              the method won't be called at all if running with
> debugging disabled.

Well, I sure hope we don't disable debugging in the iotests.  We use
assert quite a number of times there.

On the other hand, someone might want to use this outside of the iotests
but I don't even know whether that works, considering the SCM helper
program is part of the iotests.

> I suggest simply placing the os.set_inheritable() call inside
> send_fd_scm(), as close as possible to the subprocess.Popen()
> call.

Yes, I think you're right.  I didn't want to put it into send_fd_scm(),
because that method is only there to send some FD over QMP/SCM; it isn't
really supposed to send the QMP socket FD somewhere.  But sending
something over QMP/SCM is different from bequeathing something to a
child process (the socket_scm_helper).  And since send_fd_scm() needs to
bequeath the QMP socket FD to that helper, it should be responsible for
making it inheritable.

And maybe even more importantly, whether the socket allows for SCM
really has nothing to do with whether it's inheritable.  So it's
actually just wrong to put it here.

>>          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
>> +
> 
> Why not make send_fd_scm() responsible for calling
> os.set_inheritable(), making this hunk unnecessary?

My idea was: Because send_fd_scm() takes a string and not an integer.
The socket_scm_helper takes either an FD or a path, so send_fd_scm()
accepts either.

But now I realize that if we pass a path, we don't need to make the FD
inheritable.  So send_fd_scm() can check whether it's supposed to pass
an FD, and if so, make it inheritable, yes.

Max

>>          result = self.vm.send_fd_scm(str(sockfd.fileno()))
>>          self.assertEqual(result, 0, 'Failed to send socket FD')
>>  
>> -- 
>> 2.17.1
>>
>
Max Reitz Oct. 19, 2018, 9:43 a.m. UTC | #4
On 16.10.18 01:18, Cleber Rosa wrote:
> 
> 
> 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

Oops, don't know how I got that wrong.

>> 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.

Yes, will fix everywhere.

>> +            os.set_inheritable(fd, True)
>> +        except AttributeError:
>> +            pass
>> +
> 
> Doing hasattr(os, "set_inheritable") is cheaper.
> 
> - Cleber.

Nice, I'll use that then.

Max

>>          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')
>>  
>>
>
diff mbox series

Patch

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:
+            os.set_inheritable(fd, True)
+        except AttributeError:
+            pass
+
         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')