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

On 3 Jan 2023, at 5:53, 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.

Or you could restore sys/param.h to ufs_suspend.c.  Although systm.h includes
param.h, it does it too late; it is supposed to be first.  Depending on
systm.h to include param.h is therefore not reliable.  And it violates
style(9) to include both types.h and param.h.  In my opinion, restoring
param.h is the right fix.

		Mike