Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reorg: modular file reorganization of C source code - v10 #10029

Closed
wants to merge 44 commits into from

Conversation

jasonish
Copy link
Member

Continuation of #9973.

Some tests still fail:

  • The template tests, fixing the template generation will need to be done manually, so will wait til we're 100% on the layout
  • Example plugin with standalone makefile as devel headers aren't installed correctly. This will probably have to be a manual update to the Makefile(s)
  • Somethings else are bound to fail.

Tasks:

  • app-layer/
  • output/eve
  • output/
  • util/
  • util/mpm/
  • sources/
  • detect/engine
  • detect/

The clang-format check will fail. I'm using clang-format-14, the CI check
depends on clang-format-9. See: #9984

Closes #9973.

jasonish and others added 30 commits December 11, 2023 13:17
.dirstamp files are a build artifact created when GNU autoconf
projects have files spread over multiple directories.
Autoconf used to generate a config.h, but it doesn't anymore, and this
ends up ignoring files like util/config.h, which we do not want to
happen.
Add clang-format-14 as the preferred version, this is the default on
Ubuntu 22.04.
Disable clang-format around byte arrays, as they have
likely been manually formatted into something that
makes sense for humans.

This has been scripted.
@jasonish jasonish marked this pull request as draft December 11, 2023 20:37
@suricata-qa
Copy link

ERROR:

ERROR: QA failed on build_asan.

Pipeline 17008

@victorjulien
Copy link
Member

There is some logging code that relies on (parsing) the source code filename, have you looked at how that would work now?

@jasonish
Copy link
Member Author

There is some logging code that relies on (parsing) the source code filename, have you looked at how that would work now?

This logging seems fine:

Perf: af-packet: enp10s0: (W#01-enp10s0) kernel: Packets 316449, dropped 104548 [ReceiveAFPThreadExitStats:source/af-packet/source-af-packet.c:2601]
Perf: af-packet: enp10s0: (W#02-enp10s0) kernel: Packets 198017, dropped 20193 [ReceiveAFPThreadExitStats:source/af-packet/source-af-packet.c:2601]
Perf: af-packet: enp10s0: (W#03-enp10s0) kernel: Packets 596661, dropped 257360 [ReceiveAFPThreadExitStats:source/af-packet/source-af-packet.c:2601]
Perf: af-packet: enp10s0: (W#04-enp10s0) kernel: Packets 174038, dropped 11683 [ReceiveAFPThreadExitStats:source/af-packet/source-af-packet.c:2601]

is there other logging that does filename parsing?

@jasonish
Copy link
Member Author

jasonish commented Dec 12, 2023

Hmm, the -DSCFILENAME set by the Makefile is off:

gcc -DHAVE_CONFIG_H -I. -I./../libhtp/ -I/usr/include/hs -D__SCFILENAME__="parser" -march=native -Wextra -Werror-implicit-function-declaration -I/usr/include -DLOCAL_STATE_DIR="/opt/suricata/8.0.0-dev/var" -Wall -Wno-unused-parameter -Wmissing-prototypes -Wmissing-declarations -Wstrict-prototypes -Wwrite-strings -Wbad-function-cast -Wformat-security -Wno-format-nonliteral -Wmissing-format-attribute -funsigned-char -ggdb -O0 -fno-common -Wall -Wextra -Wchar-subscripts -Wno-unused-parameter -Wno-unused-function -Wno-deprecated-declarations -ggdb3 -O0 -fsanitize=address -fno-omit-frame-pointer -fno-inline -fPIC -std=c11 -march=native -I./../rust/gen -I./../rust/dist -MT app-layer/mqtt/parser.o -MD -MP -MF $depbase.Tpo -c -o app-layer/mqtt/parser.o app-layer/mqtt/parser.c &&\

And: https://github.com/OISF/suricata/blob/master/src/util-debug.c#L281

TODO:

  • Use full filename in SCFILENAME
  • Modify module name to handle paths

@jasonish
Copy link
Member Author

I think there is some more thought required here... Take 2 examples that adapt well:

  • app-layer/dnp3/parser.c
  • source/af-packet/runmode.c
    We could simply take up to the last '/' as the module name resulting in:
  • app-layer/dnp3
  • source/af-packet

But if you apply the same logic to something like util/ioctl.c we end up with:

Info: util: enp10s0: MTU 1500 [GetIfaceMTU:util/ioctl.c:99]

instead of the current module name of ioctl which makes more sense:

Info: ioctl: enp10s0: MTU 1500 [GetIfaceMTU:util/ioctl.c:99]

If there is only one / in the name maybe we could retain it, if there are 2+, we just retain the last 2 components?

@jasonish
Copy link
Member Author

Logging fixups: #10041

@jasonish jasonish closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants