Re: [PATCH v2 5/5] net/core: Allow the compiler to verify declaration and definition consistency

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Mar 25, 2019 at 07:26:42PM +0100, Sabrina Dubroca wrote:
> 2019-03-25, 09:17:23 -0700, Bart Van Assche wrote:
> > diff --git a/net/core/datagram.h b/net/core/datagram.h
> > new file mode 100644
> > index 000000000000..bcfb75bfa3b2
> > --- /dev/null
> > +++ b/net/core/datagram.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _NET_CORE_DATAGRAM_H_
> > +#define _NET_CORE_DATAGRAM_H_
> > +
> > +#include <linux/types.h>
> > +
> > +struct sock;
> > +struct sk_buff;
> > +struct iov_iter;
> > +
> > +int __zerocopy_sg_from_iter(struct sock *sk, struct sk_buff *skb,
> > +			    struct iov_iter *from, size_t length);
> > +
> > +#endif /* _NET_CORE_DATAGRAM_H_ */
> 
> That's rather ugly. Could it just be moved to an appropriate file in
> include/?

Dumping everything into widely-included files is a Bloody Bad Idea(tm);
it makes reasoning about the code much harder.

If anything, we should trim the hell out of those; details that matter
only to a well-defined subset of the kernel should be local to it.
Consider, for example, include/net/af_unix.h.  The stuff defined in
there:

Externs for functions:
unix_inflight, unix_notinflight, unix_destruct_scm, unix_gc, wait_for_unix_gc,
unix_get_socket, unix_peer_get, unix_sysctl_register, unix_sysctl_unregister,
unix_inq_len, unix_outq_len

Constants: UNIX_HASH_SIZE, UNIX_HASH_BITS

Variables: unix_tot_inflight, unix_table_lock, unix_socket_table.

Types: struct unix_address, struct unix_skb_parms, struct unix_sock.

Function-like macros and inlines: UNIXCB, unix_state_lock, unix_state_unlock,
unix_state_lock_nested, unix_sk

Convenience macro (and quite likely a namespace pollution, at that): peer_wait

Now, uses outside of net/unix/*.c and include/net/af_unix.h itself:

fs/io_uring.c:2063:     unix_destruct_scm(skb);
fs/io_uring.c:2101:             unix_inflight(fpl->user, fpl->fp[i]);
fs/io_uring.c:2105:     UNIXCB(skb).fp = fpl;
security/lsm_audit.c:323:                       struct unix_sock *u;
security/lsm_audit.c:324:                       struct unix_address *addr;
security/lsm_audit.c:354:                               u = unix_sk(sk);

and that's it.  Which is nice to realize, especially since it means that
you don't need to be concerned about something unexpected poking in
unix_socket_table internals, etc.

And actually looking at these two places outside of net/unix, you'll
see that
	* lsm_audit.c one is Linux S&M poking its fingers where they
do not belong (as usual) and, until the last cycle, screwing it up (ditto).
	* io_uring.c is a new addition, open-coding what probably
ought to be a few better-defined primitives - it's accessing the
af_unix.c guts on too low level.

Most of the af_unix.h contents definitely has no business being
visible anywhere in include/* - it should be in net/unix/ instead;
the remaining bits ought to be compactified into a sane and narrow API.
Figuring that API out is a non-trivial work, but it needs to be done.

Dumping internal details into include/* makes life much harder
when working with the code, trying to understand it, etc.
The usual reasons to separate interfaces and internals do
apply in the kernel.

Note, BTW, that stale junk (extern for a function removed at some
point, etc.) tends to stay around, confusing the hell out of readers.
And include/* tends to be considerably more sticky in that respect.

The bottom line: keep public headers tidy; internal details belong
with the code working with those.