From 15c25e91f5a59d284ef99053bcb695c9843c5e31 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 30 Mar 2023 14:41:31 +0200 Subject: [PATCH 1/4] client/tests: skip cloud-setup test for older python The test uses subprocess.Popen()'s "pass_fd" argument. That is only available since Python 3.2. Possibly it could be solved differently, but that is not implemented. Instead, skip the test. Also, socket.socket.set_inheritable() is Python 3.4. But presumably we don't need it. Fixes: d89d42bf2317 ('tests/client: test nm-cloud-setup') --- src/tests/client/test-client.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index e603203e3e..31cfbb0e1e 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2146,6 +2146,10 @@ class TestNmCloudSetup(TestNmClient): if pexpect is None: raise unittest.SkipTest("pexpect not available") + if tuple(sys.version_info[0:2]) < (3, 2): + # subprocess.Popen()'s "pass_fd" argument requires at least Python 3.2. + raise unittest.SkipTest("This test requires at least Python 3.2") + s = socket.socket() s.set_inheritable(True) s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) From a9558231cf8cdf5826829bc77a93b914b0d1fd79 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 30 Mar 2023 14:55:36 +0200 Subject: [PATCH 2/4] client/tests: drop unnecessary socket.set_inheritable() call from "test-client.py" This seems unnecessary, because we spawn the child process via subprocess.Popen and set "pass_fds". That already ensures to pass on the FD. Worse, socket.set_inheritable() is only added in Python 3.4, that means the test is gonna break for Python 3.2 and 3.3. Just avoid that by not using the unnecessary function. For the same reason, drop "inheritable=True" from os.dup2(). "inheritable=True" is already the default, but only exists since Python 3.4. Fixes: d89d42bf2317 ('tests/client: test nm-cloud-setup') --- src/tests/client/test-client.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 31cfbb0e1e..84450e2ad3 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2151,7 +2151,6 @@ class TestNmCloudSetup(TestNmClient): raise unittest.SkipTest("This test requires at least Python 3.2") s = socket.socket() - s.set_inheritable(True) s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) s.bind(("localhost", 0)) @@ -2162,7 +2161,7 @@ class TestNmCloudSetup(TestNmClient): s.listen(5) def pass_socket(): - os.dup2(s.fileno(), 3, inheritable=True) + os.dup2(s.fileno(), 3) service_path = PathConfiguration.test_cloud_meta_mock_path() env = os.environ.copy() From d533072962a494e701cdb81954c3cd52bd805eca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 30 Mar 2023 15:34:24 +0200 Subject: [PATCH 3/4] client/tests: close process stdin in test-python.py to avoid warning about leak test_ec2 (__main__.TestNmCloudSetup.test_ec2) ... /usr/lib64/python3.11/unittest/case.py:579: ResourceWarning: unclosed file <_io.BufferedWriter name=5> if method() is not None: ResourceWarning: Enable tracemalloc to get the object allocation traceback ok Fixes: d89d42bf2317 ('tests/client: test nm-cloud-setup') --- src/tests/client/test-client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 84450e2ad3..82c2919963 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2181,6 +2181,7 @@ class TestNmCloudSetup(TestNmClient): func(self) self._nm_test_post() + p.stdin.close() p.terminate() p.wait() From 342ee618c75b350cf5cccf49f2bade85c5dfa3ea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 30 Mar 2023 15:24:25 +0200 Subject: [PATCH 4/4] client/tests: don't do dup2() dance to pass file descriptor to "tools/test-cloud-meta-mock.py" "preexec_fn" is not great, because it is not generally safe in multi threaded code (and we don't know whether the test didn't start other threads already, like a GDBus worker thread). Well, it probably is safe, if you only call async signal safe code, but it's not clear what os.dup2() does under the hood. Just avoid that. We can pass on the FD directly. --- src/tests/client/test-client.py | 6 +----- tools/test-cloud-meta-mock.py | 6 ++---- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 82c2919963..b6f7cf00a4 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2160,18 +2160,14 @@ class TestNmCloudSetup(TestNmClient): # hallucinogenic substances. s.listen(5) - def pass_socket(): - os.dup2(s.fileno(), 3) - service_path = PathConfiguration.test_cloud_meta_mock_path() env = os.environ.copy() - env["LISTEN_FDS"] = "1" + env["LISTEN_FD"] = str(s.fileno()) p = subprocess.Popen( [sys.executable, service_path], stdin=subprocess.PIPE, env=env, pass_fds=(s.fileno(),), - preexec_fn=pass_socket, ) self.md_url = "http://%s:%d" % s.getsockname() diff --git a/tools/test-cloud-meta-mock.py b/tools/test-cloud-meta-mock.py index 262dc2ffb3..392955b8ad 100755 --- a/tools/test-cloud-meta-mock.py +++ b/tools/test-cloud-meta-mock.py @@ -68,11 +68,9 @@ class SocketHTTPServer(HTTPServer): # See sd_listen_fds(3) -fileno = os.getenv("LISTEN_FDS") +fileno = os.getenv("LISTEN_FD") if fileno is not None: - if fileno != "1": - raise Exception("Bad LISTEN_FDS") - s = socket.socket(fileno=3) + s = socket.socket(fileno=int(fileno)) else: addr = ("localhost", 0) s = socket.socket()