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

[Markus Armbruster]

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!