bpo-32862: Make os.dup2(fd, fd) a no-op for valid fd #5713
Conversation
os.dup2(fd, fd, inheritable=True) never changed fd inheritability, but with inheritable=False the function might fail or change it in an inconsistent manner depending on the platform.
| # dup2(fd, fd) must have no effect for a valid fd | ||
| # Issue #26935: Avoid failure due to a bionic bug | ||
| # in old Android. | ||
| if (not hasattr(sys, 'getandroidapilevel') or |
There was a problem hiding this comment.
I think this new test code should be placed in a new method. The Android checks should be put in a @unittest.skip* decorator.
| @@ -741,6 +741,9 @@ as internal buffering of data. | |||
| <fd_inheritance>` by default or non-inheritable if *inheritable* | |||
| is ``False``. | |||
|
|
|||
| If *fd* is valid and equal to *fd2*, this function has no effect | |||
There was a problem hiding this comment.
There should be a versionchanged directive.
There was a problem hiding this comment.
"this function has no effect": that's wrong, it does validate the FD which is a legit usage of this function. I would rather remove this sentence.
| @@ -3102,6 +3102,18 @@ def test_dup2(self): | |||
| self.assertEqual(os.dup2(fd, fd3, inheritable=False), fd3) | |||
| self.assertFalse(os.get_inheritable(fd3)) | |||
|
|
|||
| # dup2(fd, fd) must have no effect for a valid fd | |||
| # Issue #26935: Avoid failure due to a bionic bug | |||
There was a problem hiding this comment.
| # Issue #26935: Avoid failure due to a bionic bug | |
| # bpo-26935: Avoid failure due to a bionic bug |
I believe it's preferred to use bpo-n in code comments.
| @@ -741,6 +741,9 @@ as internal buffering of data. | |||
| <fd_inheritance>` by default or non-inheritable if *inheritable* | |||
| is ``False``. | |||
|
|
|||
| If *fd* is valid and equal to *fd2*, this function has no effect | |||
There was a problem hiding this comment.
"this function has no effect": that's wrong, it does validate the FD which is a legit usage of this function. I would rather remove this sentence.
| @@ -3102,6 +3102,18 @@ def test_dup2(self): | |||
| self.assertEqual(os.dup2(fd, fd3, inheritable=False), fd3) | |||
| self.assertFalse(os.get_inheritable(fd3)) | |||
|
|
|||
| # dup2(fd, fd) must have no effect for a valid fd | |||
There was a problem hiding this comment.
Please move this test into a separated method and emit somehow a SkipTest exception to record that the test is skipped on old Android versions.
| @@ -8041,7 +8041,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) | |||
| res = fd2; // msvcrt dup2 returns 0 on success. | |||
|
|
|||
There was a problem hiding this comment.
Please document somewhere the rationale for "fd != fd2" tests with a reference to "bpo-32862".
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
This PR is stale because it has been open for 30 days with no activity. |
os.dup2(fd, fd, inheritable=True) never changed fd inheritability,
but with inheritable=False the function might fail or change it
in an inconsistent manner depending on the platform.
https://bugs.python.org/issue32862