| Message ID | 20230119065959.3104012-5-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up includes | expand |
On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster <armbru@redhat.com> wrote: > Clean up includes so that osdep.h is included first and headers > which it implies are not included manually. > > This commit was created with scripts/clean-includes. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > bsd-user/bsd-proc.h | 4 ---- > bsd-user/qemu.h | 1 - > bsd-user/arm/signal.c | 1 + > bsd-user/arm/target_arch_cpu.c | 2 ++ > bsd-user/freebsd/os-sys.c | 1 + > bsd-user/i386/signal.c | 1 + > bsd-user/i386/target_arch_cpu.c | 3 +-- > bsd-user/main.c | 4 +--- > bsd-user/strace.c | 1 - > bsd-user/x86_64/signal.c | 1 + > bsd-user/x86_64/target_arch_cpu.c | 3 +-- > 11 files changed, 9 insertions(+), 13 deletions(-) > > diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h > index 68b66e571d..a1061bffb8 100644 > --- a/bsd-user/bsd-proc.h > +++ b/bsd-user/bsd-proc.h > @@ -20,11 +20,7 @@ > #ifndef BSD_PROC_H_ > #define BSD_PROC_H_ > > -#include <sys/types.h> > -#include <sys/stat.h> > -#include <sys/time.h> > #include <sys/resource.h> > Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill where all includes are independent, but much of that hasn't been merged to 12 yet. sys/types.h, at least, is documented as a prereq for both sys/stat.h and sys/resource.h. I know many of these are in os-dep.h, and I've had trouble in the bsd-user fork with that and the layout of the code which has arguably way too much in the .h files. Also, why didn't you move sys/resource.h and other such files to os-dep.h? I'm struggling to understand the rules around what is or isn't included where? > -#include <unistd.h> > > /* exit(2) */ > static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1) > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h > index be6105385e..0ceecfb6df 100644 > --- a/bsd-user/qemu.h > +++ b/bsd-user/qemu.h > @@ -17,7 +17,6 @@ > #ifndef QEMU_H > #define QEMU_H > > -#include "qemu/osdep.h" > #include "cpu.h" > #include "qemu/units.h" > #include "exec/cpu_ldst.h" > diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c > index 2b1dd745d1..9734407543 100644 > --- a/bsd-user/arm/signal.c > +++ b/bsd-user/arm/signal.c > @@ -17,6 +17,7 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > +#include "qemu/osdep.h" > #include "qemu.h" > > /* > diff --git a/bsd-user/arm/target_arch_cpu.c > b/bsd-user/arm/target_arch_cpu.c > index 02bf9149d5..fe38ae2210 100644 > --- a/bsd-user/arm/target_arch_cpu.c > +++ b/bsd-user/arm/target_arch_cpu.c > @@ -16,6 +16,8 @@ > * You should have received a copy of the GNU General Public License > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > + > +#include "qemu/osdep.h" > #include "target_arch.h" > > void target_cpu_set_tls(CPUARMState *env, target_ulong newtls) > diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c > index 309e27b9d6..1676ec10f8 100644 > --- a/bsd-user/freebsd/os-sys.c > +++ b/bsd-user/freebsd/os-sys.c > @@ -17,6 +17,7 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > +#include "qemu/osdep.h" > #include "qemu.h" > #include "target_arch_sysarch.h" > > diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c > index 5dd975ce56..a3131047b8 100644 > --- a/bsd-user/i386/signal.c > +++ b/bsd-user/i386/signal.c > @@ -17,6 +17,7 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > +#include "qemu/osdep.h" > #include "qemu.h" > > /* > diff --git a/bsd-user/i386/target_arch_cpu.c > b/bsd-user/i386/target_arch_cpu.c > index d349e45299..2a3af2ddef 100644 > --- a/bsd-user/i386/target_arch_cpu.c > +++ b/bsd-user/i386/target_arch_cpu.c > @@ -17,9 +17,8 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > -#include <sys/types.h> > - > #include "qemu/osdep.h" > + > #include "cpu.h" > #include "qemu.h" > #include "qemu/timer.h" > diff --git a/bsd-user/main.c b/bsd-user/main.c > index 6f09180d65..41290e16f9 100644 > --- a/bsd-user/main.c > +++ b/bsd-user/main.c > @@ -18,12 +18,10 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > -#include <sys/types.h> > -#include <sys/time.h> > +#include "qemu/osdep.h" > #include <sys/resource.h> > #include <sys/sysctl.h> > > -#include "qemu/osdep.h" > #include "qemu/help-texts.h" > #include "qemu/units.h" > #include "qemu/accel.h" > diff --git a/bsd-user/strace.c b/bsd-user/strace.c > index a77d10dd6b..96499751eb 100644 > --- a/bsd-user/strace.c > +++ b/bsd-user/strace.c > @@ -20,7 +20,6 @@ > #include <sys/select.h> > #include <sys/syscall.h> > #include <sys/ioccom.h> > -#include <ctype.h> > > #include "qemu.h" > > diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c > index c3875bc4c6..46cb865180 100644 > --- a/bsd-user/x86_64/signal.c > +++ b/bsd-user/x86_64/signal.c > @@ -16,6 +16,7 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > +#include "qemu/osdep.h" > #include "qemu.h" > > /* > diff --git a/bsd-user/x86_64/target_arch_cpu.c > b/bsd-user/x86_64/target_arch_cpu.c > index be7bd10720..1d32f18907 100644 > --- a/bsd-user/x86_64/target_arch_cpu.c > +++ b/bsd-user/x86_64/target_arch_cpu.c > @@ -17,9 +17,8 @@ > * along with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > -#include <sys/types.h> > - > #include "qemu/osdep.h" > + > #include "cpu.h" > #include "qemu.h" > #include "qemu/timer.h" > I suppose these are fine. How do I run the cleanup script? I have 2x the number of files not upstream in the bsd-user fork that I'd like to cleanup as well and they are likely a bigger mess and I'll just upstream them in the messy state unless I understand how to keep the neat :). Warner
Warner Losh <imp@bsdimp.com> writes: > On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster <armbru@redhat.com> > wrote: > >> Clean up includes so that osdep.h is included first and headers >> which it implies are not included manually. >> >> This commit was created with scripts/clean-includes. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> bsd-user/bsd-proc.h | 4 ---- >> bsd-user/qemu.h | 1 - >> bsd-user/arm/signal.c | 1 + >> bsd-user/arm/target_arch_cpu.c | 2 ++ >> bsd-user/freebsd/os-sys.c | 1 + >> bsd-user/i386/signal.c | 1 + >> bsd-user/i386/target_arch_cpu.c | 3 +-- >> bsd-user/main.c | 4 +--- >> bsd-user/strace.c | 1 - >> bsd-user/x86_64/signal.c | 1 + >> bsd-user/x86_64/target_arch_cpu.c | 3 +-- >> 11 files changed, 9 insertions(+), 13 deletions(-) >> >> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h >> index 68b66e571d..a1061bffb8 100644 >> --- a/bsd-user/bsd-proc.h >> +++ b/bsd-user/bsd-proc.h >> @@ -20,11 +20,7 @@ >> #ifndef BSD_PROC_H_ >> #define BSD_PROC_H_ >> >> -#include <sys/types.h> >> -#include <sys/stat.h> >> -#include <sys/time.h> >> #include <sys/resource.h> >> > > Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill > where all includes are independent, but much of that hasn't been merged to > 12 yet. sys/types.h, at least, is documented as a prereq for both > sys/stat.h and sys/resource.h. > > I know many of these are in os-dep.h, and I've had trouble in the bsd-user > fork with that and the layout of the code which has arguably way too much > in the .h files. No, I didn't test on FreeBSD 12. Any .c must include qemu/osdep.h *first*. Any further inclusions of headers qemu/osdep.h includes already are no-ops unless the headers in question lack multiple inclusion guards. > Also, why didn't you move sys/resource.h and other such files to os-dep.h? > I'm struggling to understand the rules around what is or isn't included > where? This series is "run scripts/clean-includes, split it into reviewable chunks, tidy up blank lines". Moving more includes into qemu/osdep.h is out of scope. I sympathize with your complaint that QEMU does too much in headers in general, and in qemu/osdep.h in particular. >> -#include <unistd.h> >> >> /* exit(2) */ >> static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1) >> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h >> index be6105385e..0ceecfb6df 100644 >> --- a/bsd-user/qemu.h >> +++ b/bsd-user/qemu.h >> @@ -17,7 +17,6 @@ >> #ifndef QEMU_H >> #define QEMU_H >> >> -#include "qemu/osdep.h" >> #include "cpu.h" >> #include "qemu/units.h" >> #include "exec/cpu_ldst.h" >> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c >> index 2b1dd745d1..9734407543 100644 >> --- a/bsd-user/arm/signal.c >> +++ b/bsd-user/arm/signal.c >> @@ -17,6 +17,7 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include "qemu/osdep.h" >> #include "qemu.h" >> >> /* >> diff --git a/bsd-user/arm/target_arch_cpu.c >> b/bsd-user/arm/target_arch_cpu.c >> index 02bf9149d5..fe38ae2210 100644 >> --- a/bsd-user/arm/target_arch_cpu.c >> +++ b/bsd-user/arm/target_arch_cpu.c >> @@ -16,6 +16,8 @@ >> * You should have received a copy of the GNU General Public License >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> + >> +#include "qemu/osdep.h" >> #include "target_arch.h" >> >> void target_cpu_set_tls(CPUARMState *env, target_ulong newtls) >> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c >> index 309e27b9d6..1676ec10f8 100644 >> --- a/bsd-user/freebsd/os-sys.c >> +++ b/bsd-user/freebsd/os-sys.c >> @@ -17,6 +17,7 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include "qemu/osdep.h" >> #include "qemu.h" >> #include "target_arch_sysarch.h" >> >> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c >> index 5dd975ce56..a3131047b8 100644 >> --- a/bsd-user/i386/signal.c >> +++ b/bsd-user/i386/signal.c >> @@ -17,6 +17,7 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include "qemu/osdep.h" >> #include "qemu.h" >> >> /* >> diff --git a/bsd-user/i386/target_arch_cpu.c >> b/bsd-user/i386/target_arch_cpu.c >> index d349e45299..2a3af2ddef 100644 >> --- a/bsd-user/i386/target_arch_cpu.c >> +++ b/bsd-user/i386/target_arch_cpu.c >> @@ -17,9 +17,8 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#include <sys/types.h> >> - >> #include "qemu/osdep.h" >> + >> #include "cpu.h" >> #include "qemu.h" >> #include "qemu/timer.h" >> diff --git a/bsd-user/main.c b/bsd-user/main.c >> index 6f09180d65..41290e16f9 100644 >> --- a/bsd-user/main.c >> +++ b/bsd-user/main.c >> @@ -18,12 +18,10 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#include <sys/types.h> >> -#include <sys/time.h> >> +#include "qemu/osdep.h" >> #include <sys/resource.h> >> #include <sys/sysctl.h> >> >> -#include "qemu/osdep.h" >> #include "qemu/help-texts.h" >> #include "qemu/units.h" >> #include "qemu/accel.h" >> diff --git a/bsd-user/strace.c b/bsd-user/strace.c >> index a77d10dd6b..96499751eb 100644 >> --- a/bsd-user/strace.c >> +++ b/bsd-user/strace.c >> @@ -20,7 +20,6 @@ >> #include <sys/select.h> >> #include <sys/syscall.h> >> #include <sys/ioccom.h> >> -#include <ctype.h> >> >> #include "qemu.h" >> >> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c >> index c3875bc4c6..46cb865180 100644 >> --- a/bsd-user/x86_64/signal.c >> +++ b/bsd-user/x86_64/signal.c >> @@ -16,6 +16,7 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include "qemu/osdep.h" >> #include "qemu.h" >> >> /* >> diff --git a/bsd-user/x86_64/target_arch_cpu.c >> b/bsd-user/x86_64/target_arch_cpu.c >> index be7bd10720..1d32f18907 100644 >> --- a/bsd-user/x86_64/target_arch_cpu.c >> +++ b/bsd-user/x86_64/target_arch_cpu.c >> @@ -17,9 +17,8 @@ >> * along with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#include <sys/types.h> >> - >> #include "qemu/osdep.h" >> + >> #include "cpu.h" >> #include "qemu.h" >> #include "qemu/timer.h" >> > > I suppose these are fine. How do I run the cleanup script? I have 2x the > number of files not upstream in the bsd-user fork that I'd like to cleanup > as well and they are likely a bigger mess and I'll just upstream them in > the messy state unless I understand how to keep the neat :). I used $ scripts/clean-includes --check-dup-head --all Which resulted in a big mess I didn't dare to submit for review :) So I split it up. Easiest way was to identify useful sets of files, add files that include headers from the set, transitively, then run $ scripts/clean-includes FILES... --check-dup-head reports possible duplicate inclusions. It is prone to false positives. That's why it merely reports them. You may want to not use it for now. There's a big usage comment at the beginning of the script. Hope this helps!
On Thu, Jan 19, 2023 at 9:42 AM Markus Armbruster <armbru@redhat.com> wrote: > Warner Losh <imp@bsdimp.com> writes: > > > On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster <armbru@redhat.com> > > wrote: > > > >> Clean up includes so that osdep.h is included first and headers > >> which it implies are not included manually. > >> > >> This commit was created with scripts/clean-includes. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> bsd-user/bsd-proc.h | 4 ---- > >> bsd-user/qemu.h | 1 - > >> bsd-user/arm/signal.c | 1 + > >> bsd-user/arm/target_arch_cpu.c | 2 ++ > >> bsd-user/freebsd/os-sys.c | 1 + > >> bsd-user/i386/signal.c | 1 + > >> bsd-user/i386/target_arch_cpu.c | 3 +-- > >> bsd-user/main.c | 4 +--- > >> bsd-user/strace.c | 1 - > >> bsd-user/x86_64/signal.c | 1 + > >> bsd-user/x86_64/target_arch_cpu.c | 3 +-- > >> 11 files changed, 9 insertions(+), 13 deletions(-) > >> > >> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h > >> index 68b66e571d..a1061bffb8 100644 > >> --- a/bsd-user/bsd-proc.h > >> +++ b/bsd-user/bsd-proc.h > >> @@ -20,11 +20,7 @@ > >> #ifndef BSD_PROC_H_ > >> #define BSD_PROC_H_ > >> > >> -#include <sys/types.h> > >> -#include <sys/stat.h> > >> -#include <sys/time.h> > >> #include <sys/resource.h> > >> > > > > Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill > > where all includes are independent, but much of that hasn't been merged > to > > 12 yet. sys/types.h, at least, is documented as a prereq for both > > sys/stat.h and sys/resource.h. > > > > I know many of these are in os-dep.h, and I've had trouble in the > bsd-user > > fork with that and the layout of the code which has arguably way too much > > in the .h files. > > No, I didn't test on FreeBSD 12. > OK. Fair enough. If it passes the CI tests, then it's likely fine (though if I hit issues, I'll submit patches). > Any .c must include qemu/osdep.h *first*. Any further inclusions of > headers qemu/osdep.h includes already are no-ops unless the headers in > question lack multiple inclusion guards. > OK. > > Also, why didn't you move sys/resource.h and other such files to > os-dep.h? > > I'm struggling to understand the rules around what is or isn't included > > where? > > This series is "run scripts/clean-includes, split it into reviewable > chunks, tidy up blank lines". Moving more includes into qemu/osdep.h is > out of scope. > OK. Fair point. I'm happy with that answer since it tells me I could move things there too, if need be. > I sympathize with your complaint that QEMU does too much in headers in > general, and in qemu/osdep.h in particular. > Yea, I'm not entirely sure it's a complaint, or if it's an observation of difficulties relative to other code bases... I go back and forth on my opinion of it... > >> -#include <unistd.h> > >> > >> /* exit(2) */ > >> static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1) > >> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h > >> index be6105385e..0ceecfb6df 100644 > >> --- a/bsd-user/qemu.h > >> +++ b/bsd-user/qemu.h > >> @@ -17,7 +17,6 @@ > >> #ifndef QEMU_H > >> #define QEMU_H > >> > >> -#include "qemu/osdep.h" > >> #include "cpu.h" > >> #include "qemu/units.h" > >> #include "exec/cpu_ldst.h" > >> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c > >> index 2b1dd745d1..9734407543 100644 > >> --- a/bsd-user/arm/signal.c > >> +++ b/bsd-user/arm/signal.c > >> @@ -17,6 +17,7 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> +#include "qemu/osdep.h" > >> #include "qemu.h" > >> > >> /* > >> diff --git a/bsd-user/arm/target_arch_cpu.c > >> b/bsd-user/arm/target_arch_cpu.c > >> index 02bf9149d5..fe38ae2210 100644 > >> --- a/bsd-user/arm/target_arch_cpu.c > >> +++ b/bsd-user/arm/target_arch_cpu.c > >> @@ -16,6 +16,8 @@ > >> * You should have received a copy of the GNU General Public License > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> + > >> +#include "qemu/osdep.h" > >> #include "target_arch.h" > >> > >> void target_cpu_set_tls(CPUARMState *env, target_ulong newtls) > >> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c > >> index 309e27b9d6..1676ec10f8 100644 > >> --- a/bsd-user/freebsd/os-sys.c > >> +++ b/bsd-user/freebsd/os-sys.c > >> @@ -17,6 +17,7 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> +#include "qemu/osdep.h" > >> #include "qemu.h" > >> #include "target_arch_sysarch.h" > >> > >> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c > >> index 5dd975ce56..a3131047b8 100644 > >> --- a/bsd-user/i386/signal.c > >> +++ b/bsd-user/i386/signal.c > >> @@ -17,6 +17,7 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> +#include "qemu/osdep.h" > >> #include "qemu.h" > >> > >> /* > >> diff --git a/bsd-user/i386/target_arch_cpu.c > >> b/bsd-user/i386/target_arch_cpu.c > >> index d349e45299..2a3af2ddef 100644 > >> --- a/bsd-user/i386/target_arch_cpu.c > >> +++ b/bsd-user/i386/target_arch_cpu.c > >> @@ -17,9 +17,8 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> -#include <sys/types.h> > >> - > >> #include "qemu/osdep.h" > >> + > >> #include "cpu.h" > >> #include "qemu.h" > >> #include "qemu/timer.h" > >> diff --git a/bsd-user/main.c b/bsd-user/main.c > >> index 6f09180d65..41290e16f9 100644 > >> --- a/bsd-user/main.c > >> +++ b/bsd-user/main.c > >> @@ -18,12 +18,10 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> -#include <sys/types.h> > >> -#include <sys/time.h> > >> +#include "qemu/osdep.h" > >> #include <sys/resource.h> > >> #include <sys/sysctl.h> > >> > >> -#include "qemu/osdep.h" > >> #include "qemu/help-texts.h" > >> #include "qemu/units.h" > >> #include "qemu/accel.h" > >> diff --git a/bsd-user/strace.c b/bsd-user/strace.c > >> index a77d10dd6b..96499751eb 100644 > >> --- a/bsd-user/strace.c > >> +++ b/bsd-user/strace.c > >> @@ -20,7 +20,6 @@ > >> #include <sys/select.h> > >> #include <sys/syscall.h> > >> #include <sys/ioccom.h> > >> -#include <ctype.h> > >> > >> #include "qemu.h" > >> > >> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c > >> index c3875bc4c6..46cb865180 100644 > >> --- a/bsd-user/x86_64/signal.c > >> +++ b/bsd-user/x86_64/signal.c > >> @@ -16,6 +16,7 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> +#include "qemu/osdep.h" > >> #include "qemu.h" > >> > >> /* > >> diff --git a/bsd-user/x86_64/target_arch_cpu.c > >> b/bsd-user/x86_64/target_arch_cpu.c > >> index be7bd10720..1d32f18907 100644 > >> --- a/bsd-user/x86_64/target_arch_cpu.c > >> +++ b/bsd-user/x86_64/target_arch_cpu.c > >> @@ -17,9 +17,8 @@ > >> * along with this program; if not, see <http://www.gnu.org/licenses/ > >. > >> */ > >> > >> -#include <sys/types.h> > >> - > >> #include "qemu/osdep.h" > >> + > >> #include "cpu.h" > >> #include "qemu.h" > >> #include "qemu/timer.h" > >> > > > > I suppose these are fine. How do I run the cleanup script? I have 2x the > > number of files not upstream in the bsd-user fork that I'd like to > cleanup > > as well and they are likely a bigger mess and I'll just upstream them in > > the messy state unless I understand how to keep the neat :). > > I used > > $ scripts/clean-includes --check-dup-head --all > > Which resulted in a big mess I didn't dare to submit for review :) So I > split it up. Easiest way was to identify useful sets of files, add > files that include headers from the set, transitively, then run > > $ scripts/clean-includes FILES... > > --check-dup-head reports possible duplicate inclusions. It is prone to > false positives. That's why it merely reports them. You may want to > not use it for now. > > There's a big usage comment at the beginning of the script. > > Hope this helps! > It does. After your changes land, I'll merge, and run this on the branch. I have a few changes queued up, and have been contemplating changes to my upstreaming workflow to speed the process along... So I'm happy with it. Thanks for the cleanup and the time to answer my questions. Reviewed-by: Warner Losh <imp@bsdimp.com> Warner
Warner Losh <imp@bsdimp.com> writes: [...] > So I'm happy with it. Thanks for the cleanup and the time to answer my > questions. > > Reviewed-by: Warner Losh <imp@bsdimp.com> Thank *you* for reviewing my patch :)
On Thu, 19 Jan 2023 at 14:42, Warner Losh <imp@bsdimp.com> wrote: > > Also, why didn't you move sys/resource.h and other such files > to os-dep.h? I'm struggling to understand the rules around what > is or isn't included where? The rough rule of thumb is that if some OS needs a compatibility fixup or workaround for a system header (eg not every mmap.h defines MAP_ANONYMOUS; on Windows unistd.h has to come before time.h) then we put that header include and the compat workaround into osdep.h. This avoids "only fails on obscure platform" issues where somebody puts a header include into some specific .c file but not the compat workaround, and it works on the Linux host that most people develop and test on and we only find the problem later. There's also no doubt some includes there for historical reasons, and some which really are "everybody needs these" convenience ones. But we should probably not add new includes to osdep.h unless they fall into the "working around system header issues" bucket. thanks -- PMM
On Fri, Jan 27, 2023 at 02:54:30PM +0000, Peter Maydell wrote: > On Thu, 19 Jan 2023 at 14:42, Warner Losh <imp@bsdimp.com> wrote: > > > > Also, why didn't you move sys/resource.h and other such files > > to os-dep.h? I'm struggling to understand the rules around what > > is or isn't included where? > > The rough rule of thumb is that if some OS needs a compatibility > fixup or workaround for a system header (eg not every mmap.h > defines MAP_ANONYMOUS; on Windows unistd.h has to come before > time.h) then we put that header include and the compat workaround > into osdep.h. This avoids "only fails on obscure platform" issues > where somebody puts a header include into some specific .c file > but not the compat workaround, and it works on the Linux host > that most people develop and test on and we only find the > problem later. > > There's also no doubt some includes there for historical > reasons, and some which really are "everybody needs these" > convenience ones. But we should probably not add new > includes to osdep.h unless they fall into the "working around > system header issues" bucket. > > thanks > -- PMM BTW maybe we should teach checkpatch about that rule: if a header is in osdep do not include it directly.
On Fri, Jan 27, 2023 at 10:01:57AM -0500, Michael S. Tsirkin wrote: > On Fri, Jan 27, 2023 at 02:54:30PM +0000, Peter Maydell wrote: > > On Thu, 19 Jan 2023 at 14:42, Warner Losh <imp@bsdimp.com> wrote: > > > > > > Also, why didn't you move sys/resource.h and other such files > > > to os-dep.h? I'm struggling to understand the rules around what > > > is or isn't included where? > > > > The rough rule of thumb is that if some OS needs a compatibility > > fixup or workaround for a system header (eg not every mmap.h > > defines MAP_ANONYMOUS; on Windows unistd.h has to come before > > time.h) then we put that header include and the compat workaround > > into osdep.h. This avoids "only fails on obscure platform" issues > > where somebody puts a header include into some specific .c file > > but not the compat workaround, and it works on the Linux host > > that most people develop and test on and we only find the > > problem later. > > > > There's also no doubt some includes there for historical > > reasons, and some which really are "everybody needs these" > > convenience ones. But we should probably not add new > > includes to osdep.h unless they fall into the "working around > > system header issues" bucket. > > > > thanks > > -- PMM > > > BTW maybe we should teach checkpatch about that rule: > if a header is in osdep do not include it directly. To be more precise, make checkpatch run clean-includes somehow? Or just make CI run clean-includes on the tree and verify result is empty? > -- > MST
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Jan 27, 2023 at 10:01:57AM -0500, Michael S. Tsirkin wrote: >> On Fri, Jan 27, 2023 at 02:54:30PM +0000, Peter Maydell wrote: >> > On Thu, 19 Jan 2023 at 14:42, Warner Losh <imp@bsdimp.com> wrote: >> > > >> > > Also, why didn't you move sys/resource.h and other such files >> > > to os-dep.h? I'm struggling to understand the rules around what >> > > is or isn't included where? >> > >> > The rough rule of thumb is that if some OS needs a compatibility >> > fixup or workaround for a system header (eg not every mmap.h >> > defines MAP_ANONYMOUS; on Windows unistd.h has to come before >> > time.h) then we put that header include and the compat workaround >> > into osdep.h. This avoids "only fails on obscure platform" issues >> > where somebody puts a header include into some specific .c file >> > but not the compat workaround, and it works on the Linux host >> > that most people develop and test on and we only find the >> > problem later. >> > >> > There's also no doubt some includes there for historical >> > reasons, and some which really are "everybody needs these" >> > convenience ones. But we should probably not add new >> > includes to osdep.h unless they fall into the "working around >> > system header issues" bucket. >> > >> > thanks >> > -- PMM >> >> >> BTW maybe we should teach checkpatch about that rule: >> if a header is in osdep do not include it directly. > > To be more precise, make checkpatch run clean-includes somehow? > Or just make CI run clean-includes on the tree and verify result > is empty? scripts/clean-includes isn't quite happy even after my series. Offenders: ebpf/rss.bpf.skeleton.h subprojects/libvduse/libvduse.h subprojects/libvhost-user/libvhost-user-glib.h subprojects/libvhost-user/libvhost-user.h target/hexagon/idef-parser/idef-parser.h target/hexagon/idef-parser/parser-helpers.h tests/fp/platform.h contrib/plugins/cache.c contrib/plugins/drcov.c contrib/plugins/execlog.c contrib/plugins/hotblocks.c contrib/plugins/hotpages.c contrib/plugins/howvec.c contrib/plugins/hwprofile.c contrib/plugins/lockstep.c linux-user/mips64/cpu_loop.c linux-user/mips64/signal.c linux-user/x86_64/cpu_loop.c linux-user/x86_64/signal.c plugins/core.c plugins/loader.c scripts/xen-detect.c subprojects/libvduse/libvduse.c subprojects/libvhost-user/libvhost-user-glib.c subprojects/libvhost-user/libvhost-user.c subprojects/libvhost-user/link-test.c target/hexagon/gen_dectree_import.c target/hexagon/gen_semantics.c target/hexagon/idef-parser/parser-helpers.c target/s390x/gen-features.c tests/migration/s390x/a-b-bios.c tests/plugin/bb.c tests/plugin/empty.c tests/plugin/insn.c tests/plugin/mem.c tests/plugin/syscall.c tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c tests/unit/test-rcu-simpleq.c tests/unit/test-rcu-slist.c tests/unit/test-rcu-tailq.c tools/ebpf/rss.bpf.c To support automatic checking, we'd have to fix the ones that need need fixing, and add the remainder to the script's XDIRREGEX.
diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h index 68b66e571d..a1061bffb8 100644 --- a/bsd-user/bsd-proc.h +++ b/bsd-user/bsd-proc.h @@ -20,11 +20,7 @@ #ifndef BSD_PROC_H_ #define BSD_PROC_H_ -#include <sys/types.h> -#include <sys/stat.h> -#include <sys/time.h> #include <sys/resource.h> -#include <unistd.h> /* exit(2) */ static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1) diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h index be6105385e..0ceecfb6df 100644 --- a/bsd-user/qemu.h +++ b/bsd-user/qemu.h @@ -17,7 +17,6 @@ #ifndef QEMU_H #define QEMU_H -#include "qemu/osdep.h" #include "cpu.h" #include "qemu/units.h" #include "exec/cpu_ldst.h" diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c index 2b1dd745d1..9734407543 100644 --- a/bsd-user/arm/signal.c +++ b/bsd-user/arm/signal.c @@ -17,6 +17,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include "qemu/osdep.h" #include "qemu.h" /* diff --git a/bsd-user/arm/target_arch_cpu.c b/bsd-user/arm/target_arch_cpu.c index 02bf9149d5..fe38ae2210 100644 --- a/bsd-user/arm/target_arch_cpu.c +++ b/bsd-user/arm/target_arch_cpu.c @@ -16,6 +16,8 @@ * You should have received a copy of the GNU General Public License * along with this program; if not, see <http://www.gnu.org/licenses/>. */ + +#include "qemu/osdep.h" #include "target_arch.h" void target_cpu_set_tls(CPUARMState *env, target_ulong newtls) diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c index 309e27b9d6..1676ec10f8 100644 --- a/bsd-user/freebsd/os-sys.c +++ b/bsd-user/freebsd/os-sys.c @@ -17,6 +17,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include "qemu/osdep.h" #include "qemu.h" #include "target_arch_sysarch.h" diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c index 5dd975ce56..a3131047b8 100644 --- a/bsd-user/i386/signal.c +++ b/bsd-user/i386/signal.c @@ -17,6 +17,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include "qemu/osdep.h" #include "qemu.h" /* diff --git a/bsd-user/i386/target_arch_cpu.c b/bsd-user/i386/target_arch_cpu.c index d349e45299..2a3af2ddef 100644 --- a/bsd-user/i386/target_arch_cpu.c +++ b/bsd-user/i386/target_arch_cpu.c @@ -17,9 +17,8 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ -#include <sys/types.h> - #include "qemu/osdep.h" + #include "cpu.h" #include "qemu.h" #include "qemu/timer.h" diff --git a/bsd-user/main.c b/bsd-user/main.c index 6f09180d65..41290e16f9 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -18,12 +18,10 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ -#include <sys/types.h> -#include <sys/time.h> +#include "qemu/osdep.h" #include <sys/resource.h> #include <sys/sysctl.h> -#include "qemu/osdep.h" #include "qemu/help-texts.h" #include "qemu/units.h" #include "qemu/accel.h" diff --git a/bsd-user/strace.c b/bsd-user/strace.c index a77d10dd6b..96499751eb 100644 --- a/bsd-user/strace.c +++ b/bsd-user/strace.c @@ -20,7 +20,6 @@ #include <sys/select.h> #include <sys/syscall.h> #include <sys/ioccom.h> -#include <ctype.h> #include "qemu.h" diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c index c3875bc4c6..46cb865180 100644 --- a/bsd-user/x86_64/signal.c +++ b/bsd-user/x86_64/signal.c @@ -16,6 +16,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include "qemu/osdep.h" #include "qemu.h" /* diff --git a/bsd-user/x86_64/target_arch_cpu.c b/bsd-user/x86_64/target_arch_cpu.c index be7bd10720..1d32f18907 100644 --- a/bsd-user/x86_64/target_arch_cpu.c +++ b/bsd-user/x86_64/target_arch_cpu.c @@ -17,9 +17,8 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ -#include <sys/types.h> - #include "qemu/osdep.h" + #include "cpu.h" #include "qemu.h" #include "qemu/timer.h"
Clean up includes so that osdep.h is included first and headers which it implies are not included manually. This commit was created with scripts/clean-includes. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- bsd-user/bsd-proc.h | 4 ---- bsd-user/qemu.h | 1 - bsd-user/arm/signal.c | 1 + bsd-user/arm/target_arch_cpu.c | 2 ++ bsd-user/freebsd/os-sys.c | 1 + bsd-user/i386/signal.c | 1 + bsd-user/i386/target_arch_cpu.c | 3 +-- bsd-user/main.c | 4 +--- bsd-user/strace.c | 1 - bsd-user/x86_64/signal.c | 1 + bsd-user/x86_64/target_arch_cpu.c | 3 +-- 11 files changed, 9 insertions(+), 13 deletions(-)