RFC on enforcing best-practice through wrappers? [PR15819, PR15722]

These two PRs are about unrelated issues, but they share a common theme:
standard APIs do not follow best practice by default, our code often
fails to jump through the required hoops to do so and, even if it did,
it would be best to factor out what it takes to follow best practice
into a single place, and use it all over, perhaps even by default.

The two cases at hand are PR15819 (poll EINTRs and we retry without
adjusting the timeout) and PR15722 (sockets are created without
CLOEXEC).

There are various ways to go about fixing this, ranging from using
__poll and __socket internal wrappers/aliases to avoid the undesirable
problems in the first place (say, having __poll never return EINTR, but
retrying with a recomputed timeout instead instead, and having __socket
always set CLOEXEC), introducing alternate functions to do so and
perhaps poisoning the original symbols so that misuses are flagged, up
to just offering wrappers that implement the best practice in internal
header wrappers or new internal headers.  AFAICT we don't seem to have
any recommended practice on how to factor out, let alone internally
enforce, this sort of best practice.

In these two patches, I've followed the last two approaches: one
introduces a static inline wrapper __poll_noeintr in sys/poll.h, that
will reenter the syscall with a recomputed timeout if it's interrupted,
and the other introduces a new header socket-cloexec.h that defines
__socket_cloexec and __socket_cloexec_nonblock static inline wrappers to
__socket that do what it takes to set these flags even when SOCK_CLOEXEC
and SOCK_NONBLOCK are not supported.

The reason for the differences is that I'd arranged for the headers to
include all headers required for functions using errno, gettimeofday,
fcntl, ioctl and whatnot to be properly parsed, and in the case of
__socket_cloexec, this became an include loop that caused some headers
to fail to be parsed correctly because referenced types hadn't been
defined after the nested #include of the header supposed to define them.

I didn't see much precedent for static inline functions in internal
headers, so I'm a bit hesitant to go either way and maybe set an
unintended precedent.  OTOH, using preprocessor macros, which would
alleviate the problem of #include loops because the macros would only be
parsed at expansion time, is more fragile to begin with, and there is
still a risk that mutually-dependent headers get included in the wrong
order and break stuff.  OYAH (On yet another hand ;-) using out-of-line
functions would introduce another layer of indirection between the
syscall and its client, which might be undesirable from a performance
perspective.

Any thoughts or recommendations on how we should address this sort of
issue, ideally in a way that makes it easier to follow best practice
than to forget to do so?

While at that, are these interfaces ones we might want to offer to our
users, as GNU extensions?

Is it overkill to offer the __socket_cloexec extra argument, on whether
or not CLOEXEC is mandatory and the whole thing should fail if we can't
make the socket cloexec?

Should __socket_cloexec_nonblock attempt a third way to make a socket
non-blocking, if the first two fail?  (absent SOCK_NONBLOCK, I'm trying
fcntl F_SETFL, like nscd, but I see sunrpc/clnt_udp.c uses the FIONBIO
ioctl)

Thanks in advance,

Here are patchlets that implement two of the alternatives I've
described, for reference.  Once we agree on one standard way to do this
sort of thing, I'll adjust the patches as needed and submit them
in a form more suitable for inclusion.

RFC on enforcing best-practice through wrappers? [PR15819, PR15722]

Commit Message

Comments

Patch