gh-157889 (os.readlink to support FDs referring to symlinks): Address feedback from MR.

* os.readlink() now returns a `str` (rather than `bytes`) when called with a file descriptor.
* os.readlink now added to `os.supports_fd` on the platforms where this feature is supported.
* Small refactor inside `os.readlink`: drop *_unavailable variables, generate error messages inline.
  Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS around each syscall site.
* Docs: Android is now mentioned as supported platform for this feature in all relevant places.

Author: OOTS

Doc/library/os.rst

@@ -2807,9 +2807,13 @@ features:
    This function can also support :ref:`paths relative to directory descriptors
    <dir_fd>`.
 
-   On MacOS and Linux, *path* can also be a file descriptor referring to a
-   symbolic link. In that case, *dir_fd* must be ``None``, and the return
-   value will be a ``bytes`` object.
+   On Linux, Android and MacOS, *path* can also be a file descriptor referring
+   to a symbolic link. In that case, *dir_fd* must be ``None``, and the return
+   value will be a string.
+   (On Linux and Android, such a file descriptor can be obtained through
+   :func:`os.open` with ``os.RDONLY | os.O_PATH | os.O_NOFOLLOW``.
+   On MacOS, this is possible by calling :func:`os.open` with
+   ``os.O_RDONLY | os.O_SYMLINK``.)
    On other operating systems, a ``NotImplementedError`` is raised if *path*
    is an integer.
 
@@ -2837,7 +2841,7 @@ features:
 
    .. versionchanged:: 3.16
       Accepts file descriptors pointing to symbolic links as *path* on
-      Linux and MacOS.
+      Linux, Android and MacOS.
 
 .. function:: remove(path, *, dir_fd=None)
 

Lib/os.py

@@ -161,6 +161,9 @@ def _add(str, fn):
         _add("HAVE_FSTATVFS", "statvfs")
     if _exists("statx"):
         _set.add(statx)
+    _add("HAVE_FREADLINK", "readlink")
+    if sys.platform in ["linux", "android"] and "O_PATH" in _globals:
+        _add("HAVE_READLINKAT", "readlink")
     supports_fd = _set
 
     _set = set()

Lib/test/test_os/test_posix.py

@@ -1908,14 +1908,19 @@ def test_readlink_with_fd(self):
             self.addCleanup(posix.unlink, fullname)
             fd = self._open_symlink_as_fd(fullname)
             self.addCleanup(os.close, fd)
-            self.assertEqual(os.readlink(fd), b"symlink")
+            self.assertEqual(os.readlink(fd), "symlink")
 
     @unittest.skipUnless(_support_readlink_with_fd,
                          "feature not supported on this platform")
     def test_readlink_with_fd_not_referring_to_symlink_throws(self):
         with self.prepare_file() as (dir_fd, name, fullname):
             fd = os.open(fullname, os.O_RDONLY)
             self.addCleanup(os.close, fd)
+            # on Linux/Android, readlinkat("", fd, ...) fails with ENOENT, which
+            # Python translates to a FileNotFoundError, a subclass of OSError.
+            # On MacOS, freadlink(fd, ...) fails with EINVAL, which gets raised
+            # as a OSError.
+            # So catching OSError here covers both cases.
             with self.assertRaises(OSError):
                 os.readlink(fd)
 
@@ -1942,6 +1947,11 @@ def test_readlink_with_fd_throws_not_implemented_error(self):
         with self.assertRaises(NotImplementedError):
             os.readlink(fd)
 
+    @unittest.skipUnless(hasattr(os, "supports_fd") and hasattr(os, "readlink"),
+                         "feature not supported on this platform")
+    def test_readlink_is_in_supports_fd_on_supported_platforms(self):
+        self.assertEqual(os.readlink in os.supports_fd, self._support_readlink_with_fd)
+
     @unittest.skipUnless(os.rename in os.supports_dir_fd, "test needs dir_fd support in os.rename()")
     def test_rename_dir_fd(self):
         with self.prepare_file() as (dir_fd, name, fullname), \

Modules/posixmodule.c

@@ -11028,77 +11028,69 @@ os_readlink_impl(PyObject *module, path_t *path, int dir_fd)
 #if defined(HAVE_READLINK)
     char buffer[MAXPATHLEN+1];
     ssize_t length;
-#ifdef HAVE_READLINKAT
-    int readlinkat_unavailable = 0;
-#endif
-#ifdef _Py_HAVE_FREADLINK
-    int freadlink_unavailable = 0;
-#endif
 
     if (path_and_dir_fd_invalid("readlink", path, dir_fd)) {
         return NULL;
     }
 
     if (path->is_fd) {
-#if defined(__APPLE__) && defined(_Py_HAVE_FREADLINK)
-        /* nop, freadlink is called below */
-#elif defined(__linux__) && defined(HAVE_READLINKAT)
-        // Linux: readlinkat(dir_fd, "", ...) reads the symbolic link
-        // pointed to by dir_fd
-        dir_fd = path->fd;
-        path->narrow = "";
+#if defined(_Py_HAVE_FREADLINK)
+        if (HAVE_FREADLINK_RUNTIME) {
+            Py_BEGIN_ALLOW_THREADS
+            length = freadlink(path->fd, buffer, MAXPATHLEN);
+            Py_END_ALLOW_THREADS
+        } else {
+            PyErr_SetString(PyExc_NotImplementedError,
+                "readlink cannot read file descriptors on this platform, "
+                "freadlink() is unavailable");
+            return NULL;
+        }
+#elif defined(HAVE_READLINKAT) && defined(O_PATH)
+        // linux/android:
+        // readlinkat(fd, "", ...) reads the link that fd refers to.
+        if (HAVE_READLINKAT_RUNTIME) {
+            Py_BEGIN_ALLOW_THREADS
+            length = readlinkat(path->fd, "", buffer, MAXPATHLEN);
+            Py_END_ALLOW_THREADS
+        } else {
+            // this should be unreachable:
+            // HAVE_READLINKAT_RUNTIME is always 1 on Linux/Android.
+            // Leaving it here as a safeguard.
+            PyErr_SetString(PyExc_NotImplementedError,
+                "readlink cannot read file descriptors on this platform, "
+                "readlinkat() is unavailable");
+            return NULL;
+        }
 #else
         PyErr_SetString(PyExc_NotImplementedError,
             "readlink cannot read file descriptors on this platform");
         return NULL;
 #endif
-    }
-
-    Py_BEGIN_ALLOW_THREADS
-#ifdef _Py_HAVE_FREADLINK
-    if (path->is_fd) {
-        if (HAVE_FREADLINK_RUNTIME) {
-            length = freadlink(path->fd, buffer, MAXPATHLEN);
-        } else {
-            freadlink_unavailable = 1;
-        }
     } else
-#endif
 #ifdef HAVE_READLINKAT
-    if (dir_fd != DEFAULT_DIR_FD || path->is_fd) {
+    if (dir_fd != DEFAULT_DIR_FD) {
         if (HAVE_READLINKAT_RUNTIME) {
+            Py_BEGIN_ALLOW_THREADS
             length = readlinkat(dir_fd, path->narrow, buffer, MAXPATHLEN);
+            Py_END_ALLOW_THREADS
         } else {
-            readlinkat_unavailable = 1;
+            argument_unavailable_error(NULL, "dir_fd");
+            return NULL;
         }
     } else
 #endif
+    {
+        Py_BEGIN_ALLOW_THREADS
         length = readlink(path->narrow, buffer, MAXPATHLEN);
-    Py_END_ALLOW_THREADS
-
-
-#ifdef _Py_HAVE_FREADLINK
-    if (freadlink_unavailable) {
-        PyErr_Format(PyExc_NotImplementedError,
-            "readlink cannot read file descriptors on this platform, "
-            "freadlink() is unavailable");
-        return NULL;
-    }
-#endif
-
-#ifdef HAVE_READLINKAT
-    if (readlinkat_unavailable) {
-        argument_unavailable_error(NULL, "dir_fd");
-        return NULL;
+        Py_END_ALLOW_THREADS
     }
-#endif
 
     if (length < 0) {
         return path_error(path);
     }
     buffer[length] = '\0';
 
-    if (PyUnicode_Check(path->object))
+    if (path->is_fd || PyUnicode_Check(path->object))
         return PyUnicode_DecodeFSDefaultAndSize(buffer, length);
     else
         return PyBytes_FromStringAndSize(buffer, length);