[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 04/19] bsd-user: Clean up includes
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v4 04/19] bsd-user: Clean up includes |
Date: |
Thu, 19 Jan 2023 17:42:14 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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!
- Re: [PATCH v4 10/19] migration: Clean up includes, (continued)
- [PATCH v4 14/19] block: Clean up includes, Markus Armbruster, 2023/01/19
- [PATCH v4 03/19] scripts/clean-includes: Skip symbolic links, Markus Armbruster, 2023/01/19
- [PATCH v4 02/19] scripts/clean-includes: Don't claim duplicate headers found when not, Markus Armbruster, 2023/01/19
- [PATCH v4 04/19] bsd-user: Clean up includes, Markus Armbruster, 2023/01/19
- Re: [PATCH v4 04/19] bsd-user: Clean up includes, Peter Maydell, 2023/01/27
- Re: [PATCH v4 04/19] bsd-user: Clean up includes, Michael S. Tsirkin, 2023/01/27
- Re: [PATCH v4 04/19] bsd-user: Clean up includes, Michael S. Tsirkin, 2023/01/28
- Re: [PATCH v4 04/19] bsd-user: Clean up includes, Markus Armbruster, 2023/01/30
[PATCH v4 15/19] accel: Clean up includes, Markus Armbruster, 2023/01/19
[PATCH v4 05/19] crypto: Clean up includes, Markus Armbruster, 2023/01/19