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

[Markus Armbruster]

"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.