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 - v11 #10041

Draft
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

jasonish
Copy link
Member

Previous PR: #10029
Changes from previous PR:

  • Better handle the logging of module names

File reorganization tasks:

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

Module Name Logging

  • For files/modules that are in directories, new module logging transformation is done. If only one path separator, the name is logged as is. Example: util/ioctl -> util/ioctl. In current git master this would be logged as ioctl.
  • If more than one path separator is found, the last component is dropped, for example:
    • source/af-packet/runmode -> source/af-packet
    • app-layer/dnp3/parser -> app-layer/dnp3
  • Handling of files in the top source directory are unchanged

Known Failures

  • Templates fail, will fix when the layout of files is approved
  • example plugins fail, will fix when the layout of files is approved
  • github-ci clang-formatting check, this branch is done with clang-format-14

jasonish and others added 30 commits December 12, 2023 14:49
Add clang-format-14 as the preferred version, this is the default on
Ubuntu 22.04.
Update the formatting CI job to Ubuntu 22.04 to get a newer version of
clang-format, in this case clang-format-14.
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.
.dirstamp files are a build artifact created when GNU autoconf
projects have files spread over multiple directories.
If a module has path separators, base the module name on the path.  If
only 1 "/" (util/ioctl) log the full path as the module.  If more
(app-layer/dnp3/parser) drop the last component so a module name might
be something like "app-layer/dnp3", or "source/af-packet".

This is primarily in prep of the modular re-organization.

Additionally, actually use dn_len when building the log message to allow
for truncation of names.
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
Copy link
Member Author

@jlucovsky Can you look at commit 419cb14 please.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17023

@catenacyber
Copy link
Contributor

I think this is a good idea globally.

Is there something to do with compiling the different directories as different relatively independent libraries ?

@jasonish
Copy link
Member Author

Is there something to do with compiling the different directories as different relatively independent libraries ?

No. That is a style seen, but I think its more of a relic of the past. It just adds time to the build if not needed.

@catenacyber
Copy link
Contributor

No. That is a style seen, but I think its more of a relic of the past. It just adds time to the build if not needed.

😮
As I see it, it helps for library usage by having smaller reusable components...

@jasonish
Copy link
Member Author

As I see it, it helps for library usage by having smaller reusable components...

Perhaps if written that way from the start. I don't know if our code maps to that strategy very well though.

There's also this: https://accu.org/journals/overload/14/71/miller_2004/#:~:text=The%20use%20of%20recursive%20make%20affects%20both%20phases%20of%20the,DAG%20in%20an%20inappropriate%20order.

@jasonish
Copy link
Member Author

As I see it, it helps for library usage by having smaller reusable components...

Something to keep in mind for new sub-systems I suppose.. Do they make sense to have as a standalone library? Same with Rust, does something new make sense as a standalone crate?

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17023

@catenacyber
Copy link
Contributor

Now, I wonder if we just should move C code to rust where it is more organized...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

3 participants