include: Include <sys/select.h> when building for modern Linux.

[vonosmas]

Fix curl compilation errors when building on Linux with
LLVM-libc (libc.llvm.org). It provides fd_set type (and
associated macro) in <sys/select.h> according to POSIX.

curl uses fd_set type (and associated FD* macro) that are
provided by <sys/select.h> according to POSIX. In practice,
some libc implementations (notably, glibc on Linux) transitively
include this header from <sys/types.h>, but other implementations
with stricter POSIX compliance may not do that
(see example of LLVM-libc above).

Fix curl compilation under LLVM-libc by including <sys/select.h>
in public <curl.h> when building for Linux and _POSIX_C_SOURCE is set
(by the user or by system libc) to be at least 200112L, referencing
the POSIX standard that added <sys/select.h> / fd_set.

[bagder]

I would like you to be more specific for exactly which systems these changes are necessary?

[vonosmas]

I would like you to be more specific for exactly which systems these changes are necessary?

Sure. I was building for Linux (x86-64, but same would be for other platforms
like aarch64 and riscv), when building with LLVM-libc.

Updated the PR description to be more specific.

[bagder]

The libcurl source code includes <sys/select.h> already in the source files that need it. If that is missed somewhere, the include should be put in that file and not in this generic header file.

[vonosmas]

I think putting includes in source files will be problematic:

• curl_multi_fdset function in public API:

  curl/include/curl/multi.h

  Lines 157 to 161 in 6c04b42

  	CURL_EXTERN CURLMcode curl_multi_fdset(CURLM *m,
  	fd_set *read_fd_set,
  	fd_set *write_fd_set,
  	fd_set *exc_fd_set,
  	int *max_fd);

  has fd_set * arguments. I believe we can't just "forward-declare" those as, unlike structs, fd_set is a typedef (https://man7.org/linux/man-pages/man2/select.2.html), so we need to know what it is when curl_multi_fdset declaration is first encountered. The only way it would work is if public users will make sure to include <sys/select.h> before including CURL headers, which is probably a big if.
• same goes for the internal usage in cshutdn:

  curl/lib/cshutdn.h

  Lines 97 to 99 in 6c04b42

  	void Curl_cshutdn_setfds(struct cshutdn *cshutdn,
  	fd_set *read_fd_set, fd_set *write_fd_set,
  	int *maxfd);

  where we need to either include <sys/select.h> in a cshutdn.h header, or chase all source files where this header is (transitively) included, regardless of whether the function is used.

[bagder]

we need to either include <sys/select.h> in a cshutdn.h header

I think that looks like the correct place

Also: I think getting a CI job in place that builds curl using a setup like this could be valuable.

[vonosmas]

Thank you! I've moved the <sys/select.h> inclusion to lib/cshutdn.h (and had it include "curl_setup.h" to get HAVE_SYS_SELECT_H configuration).

That said, it doesn't help with the need to have fd_set definition in curl_multi_fdset in public API, so I had to preserve the change in include/curl/curl.h (side note: that change by itself would be enough, since I think public API headers are included everywhere in lib implementation through curl_setup.h).

I believe we can avoid using the libc-specific defines if we just allow <sys/select.h> inclusion on all Linux platforms by adding defined(__linux__) to the long list of conditions used currently, unless libc5 compatibility is still a requirement - glibc 2.0 onwards (post-1997), and alternative libc implementations all ship it; it's standardized since POSIX.1-2001.

Speaking of the CI setup - I think LLVM-libc specifically is still short functionality-wise from being able to use it conveniently (as a system libc on a modern Linux version) in self-contained CI setups suitable for Github actions.

[bagder]

that change by itself would be enough

Then why suggest the additional edit? I take it you're not actually building curl against this libc?

[bagder]

it's standardized since POSIX.1-2001.

So maybe instead of checking for __linux__ we should check for _POSIX_VERSION >= 200112L ?

(reference)

[vonosmas]

that change by itself would be enough

Then why suggest the additional edit? I take it you're not actually building curl against this libc?

I was building curl against this libc, but that's irrelevant -- I didn't realize at first that a change to the public <curl.h> header would be enough by itself, as we can confidently rely on it being included in library's private sources/headers as well. As a result, the discussion got sidetracked -- I apologize for that. Reverted the library header change.

[vonosmas]

So maybe instead of checking for __linux__ we should check for _POSIX_VERSION >= 200112L ?

(reference)

Thank you for suggesting relying on POSIX feature test macro instead. I looked into that, and the issue with _POSIX_VERSION is that it's defined in <unistd.h>, which is itself not available on all Platforms... (e.g. on Windows). That said, I think a feasible alternative can be an "input macro" _POSIX_C_SOURCE (reference) which is either set in the compilation environment by the user (to specify which POSIX they're building for), or set to a reasonable default by the libc implementations themselves. However, in the current version of the patch I've still constrained this to Linux only, as I don't think it's uncommon for developers for MinGW, RTOS, or other systems forcefully define _POSIX_C_SOURCE to get more library features they want.

[bagder]

Thanks!