Another sensible behaviour would be to ignore dir_fd completely, but I think raising an error here is the better alternative.

This just so happened - maybe a str is more reasonable?

Yes, readlins returns str (with surogatepass encoding, so you can get the bytes out losslessly).

Updated: The implementation returns a "string" (via PyUnicode_DecodeFSDefaultAndSize), the tests check for equality with a string ("symlink" rather than b"symlink"), and the docs are updated, too.

Yes, readlins returns str (with surogatepass encoding, so you can get the bytes out losslessly).

I think we only want _Py_HAVE_FREADLINK here -- if any other platform has it, it's the function to call.

Removed the defined(__APPLE__) check.

It would be nice to document how to get such a file descriptor (O_SYMLINK and O_PATH|O_NOFOLLOW).

Added this to the docs.

AFAICS, we don't need the *_unavailable variables. The pattern is sometimes used because we can't raise exceptions inside Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS, but here, moving the Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS to tightly wrap each freadlink/readlinkat/readlink call would allow setting the exception & returning direclty in the else blocks.

Done. I dropped the freadlink_unavailable variable and the error is now raised directly in the else branch.

I also "inlined" all the invocations of Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS, each syscall (freadlink, readlinkat in the path->is_fd case, readlinkat in the old dir_fd case, readlink) are now directly surrounded by those macros.

This is technically a bit of a refactor and slightly out-of-scope of the issue - but I hope it's okay to piggy-back this refactor onto this MR.

Putting the values in these variables is misleading.
Instead, could you add a separate block below with a call to readlinkat(path->fd, "", ...) & the error handling?

Done. We now have a if (path->is_fd) { ... } else if (path->dir_fd != DEFAULT_DIR_FD) { ... } else { ... } structure.

The different implementations of this feature (freadlink on MacOS, readlinkat on Linux/Android, NotImplementedError on Android) are now all inside the if (path->is_fd) { ... } branch.

I think it would be useful to make this a configure variable, something like _Py_READLINKAT_SUPPORTS_EMPTY_PATH, only set on Linux.
Then, when someone ports CPython to another system that uses this (or to ancient Linux that doesn't), they can just change pyconfig.h or the configure script.

Alternately, use defined(HAVE_READLINKAT) && defined(O_PATH). O_PATH and empty pathname support were added in Linux 2.6.39; they look like it's essentially a single feature.

I tried with defined(HAVE_READLINKAT) && defined(O_PATH), but that broke on the emscripten build. (Apparently both Macros are defined there even though readlink is not supported on that platform AFAIK.)

So now _Py_READLINKAT_SUPPORTS_EMPTY_PATH is set automatically on Linux and Android (that have O_PATH) above. Anyone who really wants it can still force that variable.

I didn't add anything about this to the autoconfig part - I hope that's okay. If you want it threaded through/into the configure.ac somehow, I can look into that.

Could you add Android to the other docs?

Done.

One open question is whether os.readlink() should be added to supports_fd. I don't have an opinion on that, happy to take others' lead here.

I'd say add them. O_SYMLINK/O_PATH|O_NOFOLLOW are the way to get fds referring to a symlink on their respective platform, so this can be just os.readlink in os.supports_dir_fd.
(Yes, I changed my mind after going through the code.)

Then, please also test that os.readlink in os.supports_dir_fd holds on MacOS, Linux & Android.

Done. os.readlink is now added automatically to os.supports_fd. I also added a test that checks presence/absence on all platforms where both os.readlink and os.supports_fd are defined.

_support_readlink_with_fd should now be equivalent to hasattr(os, 'readlink') and (os.readlink in os.supports_fd). Or more fancy, (getattr(os, 'readlink', None) in os.supports_fd)
The in expression can be used directly, as in the test above; no need for a _support_readlink_with_fd wariable.

I'm updating this, but I'd prefer to keep the _support_readlink_with_fd variable (just for avoiding code/logic duplication).

As a side effect, I'm now removing the self.assertEqual(os.readlink in os.supports_fd, self._support_readlink_with_fd) test that you asked for, since this test would be tautological. (It currently sits behind a @unittest.skipUnless(hasattr(os, "readlink")) guard.)

Okay, rather than removing the test completely I added something closer to what you originally asked for:

    @unittest.skipUnless(sys.platform in ["linux", "android", "darwin", "ios"],
                         "feature not supported on this platform")
    def test_readlink_is_marked_as_supports_fd(self):
        self.assertIn(os.readlink, os.supports_fd)

According to other tests, hasattr(os, "supports_fd") test is not needed, the attribute can always be used.

Sorry, I missed this comment. I just pushed an update: 6c8f5f1

Originally I wanted to be careful (because the supports_fd set is only defined within an if _exists("_have_functions") guard), but it appears that the _have_functions list is defined unconditionally (at least in the posixmodule.c).

_support_readlink_with_fd should now be equivalent to hasattr(os, 'readlink') and (os.readlink in os.supports_fd). Or more fancy, (getattr(os, 'readlink', None) in os.supports_fd)
The in expression can be used directly, as in the test above; no need for a _support_readlink_with_fd wariable.