From nobody Wed Jan 4 01:51:45 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4NmszL0JX4z2pSrp for ; Wed, 4 Jan 2023 01:51:58 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4NmszK5LZ9z3rLR for ; Wed, 4 Jan 2023 01:51:57 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ej1-x633.google.com with SMTP id ud5so79411718ejc.4 for ; Tue, 03 Jan 2023 17:51:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ds38/WMaNv2/gTiSeS1/7sluYq7cJo1cJaWEbD1QsoQ=; b=qzIXfrncmwwE/lBN6HzjHH6v5rPmpO7sw0Xq0n2gTmxUOZG1t43RCurafuIHf396H7 hx9HYB0JelcAlGJyQ47tXQA1oURT2HxUdOsf2oR76tBkO/i4t9uXK+g+/8ICQl89XwXf VWZaiyDRlUUfbNMFdQmcbGdTNVzKtvrIStuJjwWCV4P2gmgz8v9mtFB4RhD9EZs0Z5w4 aa/+PY527KGZuA1VCeUPKrli8HH1hsQ95BXoTm91MmES6jrQyYLu+7ciQFU5NNBjfwJA G1T9nJmJm2mz07XSkjmWso9iOlpuZb4jlZNjCsrR1mETzSfmQBQ4FnWPwwHevVddFyGk rnUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ds38/WMaNv2/gTiSeS1/7sluYq7cJo1cJaWEbD1QsoQ=; b=scSTdpvzgKxNVdIrhPV1BdYYObuLZ2JebBO2Ze1FzxQqjWwuoGNLoLVLnQ/R2QDmOx 3Cp3yQnnPsvJlY+C2XGvklsOm8JVulke2WOugCa/BC4tBro8B9vqrw/og3ui0Snwdpc+ MB0oDXv+AOt4+WeM9k8JEOGcwUY2XCfWiJVhywCLg6CeFZo1g5xK2Zljho3qoINQcaeU kb9yeTd3AuqfbEJHiEbRYMRaU4cJ39DUJmS+AGGCPvQHv+1nEIx5EKUbe5CZ43zPp0Ar GkhIuUu4ZZxpF/8Fuquy2LPuR11xievNsZyfiYLzCfwrQslF+24qGrxPVu7c5B5j3O3O ZgZw== X-Gm-Message-State: AFqh2krZB9IhudgTyr4dB5Hv4J6D8fNMVJPqyFYkLWQqwX1Bqw9moC45 mS5OSgUaVBmIPSyyOUFQIfi91L4e1JpVPrnVB0C+xQ== X-Google-Smtp-Source: AMrXdXuiQfQ9+CjR3RiPOgBK5ye0AKOmKCHus3M4nA4FkbHqsdwV3Jr23qH9audJPWhTf+FcSCP7j2P0AOVd8z1yRjI= X-Received: by 2002:a17:906:4a8f:b0:7c0:e6d8:3e82 with SMTP id x15-20020a1709064a8f00b007c0e6d83e82mr3047662eju.451.1672797116343; Tue, 03 Jan 2023 17:51:56 -0800 (PST) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202212292056.2BTKuOIu047460@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Tue, 3 Jan 2023 18:51:45 -0700 Message-ID: Subject: Re: git: 68912701700c - main - ffs_suspend.c: clean up includes To: Mark Johnston Cc: Konstantin Belousov , Mateusz Guzik , src-committers , "" , dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="000000000000e6ba9005f1666d15" X-Rspamd-Queue-Id: 4NmszK5LZ9z3rLR X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --000000000000e6ba9005f1666d15 Content-Type: text/plain; charset="UTF-8" On Tue, Jan 3, 2023, 5:52 PM Mark Johnston wrote: > On Tue, Jan 03, 2023 at 01:53:27PM +0200, Konstantin Belousov wrote: > > On Mon, Jan 02, 2023 at 06:29:57PM +0100, Mateusz Guzik wrote: > > > On 12/29/22, Konstantin Belousov wrote: > > > > The branch main has been updated by kib: > > > > > > > > URL: > > > > > https://cgit.FreeBSD.org/src/commit/?id=68912701700ca3230f3e2d4b7858a038f884a327 > > > > > > > > commit 68912701700ca3230f3e2d4b7858a038f884a327 > > > > Author: Konstantin Belousov > > > > AuthorDate: 2022-12-28 18:17:53 +0000 > > > > Commit: Konstantin Belousov > > > > 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 > > > > __FBSDID("$FreeBSD$"); > > > > > > > > -#include > > > > #include > > > > #include > > > > -#include > > > > -#include > > > > -#include > > > > #include > > > > +#include > > > > #include > > > > +#include > > > > #include > > > > +#include > > > > > > > > #include > > > > > > > > > > > > > > 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. > > All machine/atomic.h include atomic_common.h, which includes > sys/types.h. Admittedly this inlcude was added quite recently, but it > means that systm.h is always polluted, so it seems reasonable to me. > > > I would prefer #3, it seems, but > > want to ensure that there is a consensus about the addition to > sys/systm.h. > > As Mike notes, systm.h already includes param.h, but it happens too late. > > Most of systm.h is _KERNEL-only, so I can't see why the includes at the > beginning are unguarded. Does userspace (or the boot loader) rely on > systm.h for anything? Perhaps we should guard the whole file with > _KERNEL and put includes in the right order. That would also solve the > problem. > I prefer mark's review that makes the atomic_san.h file self contained. Warner > --000000000000e6ba9005f1666d15 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Jan 3, 2023, 5:52 PM Mark Johnston <markj@freebsd.org> wrote:
On Tue, Jan 03, 2023 at 01:53:27PM +0200, Konst= antin 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:<= br> > > > The branch main has been updated by kib:
> > >
> > > URL:
> > > https://cgit.FreeBSD.org/src/commit/?id=3D68912701700ca3230f3e2d4b7= 858a038f884a327
> > >
> > > commit 68912701700ca3230f3e2d4b7858a038f884a327
> > > Author:=C2=A0 =C2=A0 =C2=A0Konstantin Belousov <kib@FreeB= SD.org>
> > > AuthorDate: 2022-12-28 18:17:53 +0000
> > > Commit:=C2=A0 =C2=A0 =C2=A0Konstantin Belousov <kib@FreeB= SD.org>
> > > CommitDate: 2022-12-29 20:55:39 +0000
> > >
> > >=C2=A0 =C2=A0 =C2=A0ffs_suspend.c: clean up includes
> > >
> > >=C2=A0 =C2=A0 =C2=A0Order includes alphabetically.
> > >=C2=A0 =C2=A0 =C2=A0Remove unneeded sys/param.h, it is alread= y included by sys/systm.h.
> > >
> > >=C2=A0 =C2=A0 =C2=A0Reviewed by:=C2=A0 =C2=A0 mckusick
> > >=C2=A0 =C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0The FreeBSD Fou= ndation
> > >=C2=A0 =C2=A0 =C2=A0MFC after:=C2=A0 =C2=A0 =C2=A0 1 week
> > >=C2=A0 =C2=A0 =C2=A0Differential revision:=C2=A0 https://reviews.freebsd.org/D37896
> > > ---
> > >=C2=A0 sys/ufs/ffs/ffs_suspend.c | 7 +++----
> > >=C2=A0 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/sys/ufs/ffs/ffs_suspend.c b/sys/ufs/ffs/ffs_sus= pend.c
> > > index d13097109758..e7c976b6e921 100644
> > > --- a/sys/ufs/ffs/ffs_suspend.c
> > > +++ b/sys/ufs/ffs/ffs_suspend.c
> > > @@ -33,15 +33,14 @@
> > >=C2=A0 #include <sys/cdefs.h>
> > >=C2=A0 __FBSDID("$FreeBSD$");
> > >
> > > -#include <sys/param.h>
> > >=C2=A0 #include <sys/systm.h>
> > >=C2=A0 #include <sys/buf.h>
> > > -#include <sys/ioccom.h>
> > > -#include <sys/mount.h>
> > > -#include <sys/vnode.h>
> > >=C2=A0 #include <sys/conf.h>
> > > +#include <sys/ioccom.h>
> > >=C2=A0 #include <sys/jail.h>
> > > +#include <sys/mount.h>
> > >=C2=A0 #include <sys/sx.h>
> > > +#include <sys/vnode.h>
> > >
> > >=C2=A0 #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 &#= 39;uint8_t'
> > ATOMIC_SAN_FUNCS(char, uint8_t);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 ^
> > it bisects to this commit
> >
> So the problem is that sys/systm.h includes machine/atomic.h which alw= ays
> 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, an= d
> 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 someti= mes,
> when the right kernel options are used.

All machine/atomic.h include atomic_common.h, which includes
sys/types.h.=C2=A0 Admittedly this inlcude was added quite recently, but it=
means that systm.h is always polluted, so it seems reasonable to me.

> I would prefer #3, it seems, but
> want to ensure that there is a consensus about the addition to sys/sys= tm.h.

As Mike notes, systm.h already includes param.h, but it happens too late.
Most of systm.h is _KERNEL-only, so I can't see why the includes at the=
beginning are unguarded.=C2=A0 Does userspace (or the boot loader) rely on<= br> systm.h for anything?=C2=A0 Perhaps we should guard the whole file with
_KERNEL and put includes in the right order.=C2=A0 That would also solve th= e
problem.

I prefer mark's review that makes the atomic_san.h file self = contained.=C2=A0

Warner= =C2=A0
--000000000000e6ba9005f1666d15--