Re: git: 68912701700c - main - ffs_suspend.c: clean up includes

On 28 Feb 2023, at 12:55, John Baldwin wrote:

> On 1/3/23 3:53 AM, Konstantin Belousov wrote:
>> On Mon, Jan 02, 2023 at 06:29:57PM +0100, Mateusz Guzik wrote:
>>> On 12/29/22, Konstantin Belousov <kib@freebsd.org> wrote:
>>>> The branch main has been updated by kib:
>>>>
>>>> URL:
>>>> https://cgit.FreeBSD.org/src/commit/?id=68912701700ca3230f3e2d4b7858a038f884a327
>>>>
>>>> commit 68912701700ca3230f3e2d4b7858a038f884a327
>>>> Author:     Konstantin Belousov <kib@FreeBSD.org>
>>>> AuthorDate: 2022-12-28 18:17:53 +0000
>>>> Commit:     Konstantin Belousov <kib@FreeBSD.org>
>>>> CommitDate: 2022-12-29 20:55:39 +0000
>>>>
>>>>      ffs_suspend.c: clean up includes
>>>>
>>>>      Order includes alphabetically.
>>>>      Remove unneeded sys/param.h, it is already included by sys/systm.h.
>>>>
>>>>      Reviewed by:    mckusick
>>>>      Sponsored by:   The FreeBSD Foundation
>>>>      MFC after:      1 week
>>>>      Differential revision:  https://reviews.freebsd.org/D37896
>>>> ---
>>>>   sys/ufs/ffs/ffs_suspend.c | 7 +++----
>>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/sys/ufs/ffs/ffs_suspend.c b/sys/ufs/ffs/ffs_suspend.c
>>>> index d13097109758..e7c976b6e921 100644
>>>> --- a/sys/ufs/ffs/ffs_suspend.c
>>>> +++ b/sys/ufs/ffs/ffs_suspend.c
>>>> @@ -33,15 +33,14 @@
>>>>   #include <sys/cdefs.h>
>>>>   __FBSDID("$FreeBSD$");
>>>>
>>>> -#include <sys/param.h>
>>>>   #include <sys/systm.h>
>>>>   #include <sys/buf.h>
>>>> -#include <sys/ioccom.h>
>>>> -#include <sys/mount.h>
>>>> -#include <sys/vnode.h>
>>>>   #include <sys/conf.h>
>>>> +#include <sys/ioccom.h>
>>>>   #include <sys/jail.h>
>>>> +#include <sys/mount.h>
>>>>   #include <sys/sx.h>
>>>> +#include <sys/vnode.h>
>>>>
>>>>   #include <security/mac/mac_framework.h>
>>>>
>>>>
>>>
>>> tinderbox fails the KCSAN et al kernels:
>>>
>>> In file included from /usr/src/sys/ufs/ffs/ffs_suspend.c:36:
>>> In file included from /usr/src/sys/sys/systm.h:44:
>>> In file included from ./machine/atomic.h:73:
>>> /usr/src/sys/sys/atomic_san.h:117:24: error: unknown type name 'uint8_t'
>>> ATOMIC_SAN_FUNCS(char, uint8_t);
>>>                         ^
>>> it bisects to this commit
>>>
>> So the problem is that sys/systm.h includes machine/atomic.h which always
>> had the pre-requisite sys/types.h, and this is documented in atomic(9).
>> But sys/atomic_san.h uses C89 types in prototypes, not just macros, and
>> this breaks for real.
>>
>> I can
>> 1. Just add sys/types.h to ufs_suspend.c (I prefer not)
>> 2. Add sys/types.h to sys/atomic_san.h.
>> 3. Add sys/types.h to sys/systm.h.
>>
>> IMO #2 is not the best solution, since it pollutes systm.h only sometimes,
>> when the right kernel options are used.  I would prefer #3, it seems, but
>> want to ensure that there is a consensus about the addition to sys/systm.h.
>
> FYI, I got a different ream of breakages downstream in CheriBSD due to this
> commit removing param.h as well.  I also only ever recalled Bruce telling me
> to do either <sys/types.h> or <sys/param.h> first (and only one of them).
> I don't recall any special rules for <sys/systm.h>.
>
> CheriBSD has a local diff that tries to sort headers in systm.h which is why
> we get larger breakage:
>
> --- a/sys/sys/systm.h
> +++ b/sys/sys/systm.h
> @@ -41,13 +41,22 @@
>  #define        _SYS_SYSTM_H_
>   #include <sys/cdefs.h>
> -#include <machine/atomic.h>
> -#include <machine/cpufunc.h>
>  #include <sys/callout.h>
>  #include <sys/kassert.h>
>  #include <sys/queue.h>
>  #include <sys/stdint.h>                /* for people using printf mainly */
>  +#include <machine/atomic.h>
> +#include <machine/cpufunc.h>
>
> I think if we want <sys/systm.h> to be self-contained we should swap <sys/cdefs.h>
> for <sys/types.h>.  I think moving the <machine/*> includes down would also be
> more consistent with style(9) as well.

I prefer retaining <sys/param.h> or <sys/types.h> to be the first include in kernel
files, as has long been traditional.  systm.h didn’t originally include param.h,
and param.h isn’t anywhere near the top of systm.h.  I’m sure it was just added
to unbreak some file that didn’t include param.h when a dependency on MAXCPU crept
in.  It looks like 15 or more headers in sys/ include <sys/param.h>; systm.h is
not really special in that regard.  Finally, style(9) documents use of either
<sys/param.h> or <sys/types.h> first, then <sys/systm.h> if needed.

		Mike