Re: [PATCH v4 04/19] bsd-user: Clean up includes

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 8514 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 12849 bytes --]