Include less v5

[catenacyber]

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/945

Describe changes:

• Rework includes as per cppclean
• util: fix integer warnings in profiling

Modifies #7643 with adding CI check ! using the patched cppclean

This PR adds about one hundred lines #include "detect-engine-build.h" because it is being used in util tests for SigGroupBuild and such... Any thoughts on that ?

[codecov]

Codecov Report

Merging #7644 (a21a9ce) into master (f8bf581) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7644      +/-   ##
==========================================
+ Coverage   75.75%   75.82%   +0.06%     
==========================================
  Files         659      659              
  Lines      185743   185744       +1     
==========================================
+ Hits       140713   140832     +119     
+ Misses      45030    44912     -118     

Flag	Coverage Δ
fuzzcorpus	60.30% <ø> (+0.15%)	⬆️
suricata-verify	52.50% <ø> (+0.07%)	⬆️
unittests	60.71% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[suricata-qa]

Information: QA ran without warnings.

Pipeline 8325

[victorjulien]

it does feel a bit weird to include rust.h here when the parser doesn't use rust

[catenacyber]

But it does : AppLayerTxData is defined in rust

[victorjulien]

this seems out of place here?

[victorjulien]

nm I see it now

[catenacyber]

Needed for SLL_HEADER_LEN

Maybe SLL_HEADER_LEN can be defined some place else so that util does not depend on decode...
But we may want to do that later

[victorjulien]

nit: empty guard, will remove

[catenacyber]

Can do as well

[victorjulien]

Think this looks really good. The only thing that feels a bit like a hack is the including of rust.h in places where no Rust is used, so it appears to be using rust.h as a common header there. But maybe rust.h should in fact be merged into suricata.h?

Anyway, I feel this should not block merge and we can do further rounds of cleanups for this.

[catenacyber]

Think this looks really good. The only thing that feels a bit like a hack is the including of rust.h in places where no Rust is used, so it appears to be using rust.h as a common header there. But maybe rust.h should in fact be merged into suricata.h?

Where is rust.h included beyond enip which looks hacky ? I may have missed some, but I think rust.h is needed for its definition of AppLayerTxData

The best way forward would to have multiple rust-X.h generated by cbindgen, but I do not think it will be easy

[victorjulien]

Merged in #7653, thanks!

[catenacyber]

Now I wonder what share of current PRs will fail CI because of this new check ;-)

[jasonish]

The best way forward would to have multiple rust-X.h generated by cbindgen, but I do not think it will be easy

I don't think this is easy. Thats part of the reason why I was doing the experimental config crate as its own crate, 1) So it could be used independently of the rest of Suricata, and 2) so I could easily give it its own self contained header file.

[catenacyber]

Should we make more suricata crates ?

[jasonish]

Should we make more suricata crates ?

Yeah, probably not a bad idea. But it might still be too early to decide what goes where. Would probably end up with a suricata_types crate just for types maybe. Not doing it now leads to some things that might be hard to deal with, doing it too soon could also lead to complications I think. Or we embrace the super header.

[jufajardini]

Now I wonder what share of current PRs will fail CI because of this new check ;-)

:D: Failing because it doesn't recognize a macro called inside a macro, here: https://github.com/jufajardini/suricata/runs/7733918797?check_suite_focus=true

[catenacyber]

Now I wonder what share of current PRs will fail CI because of this new check ;-)

:D: Failing because it doesn't recognize a macro called inside a macro, here: https://github.com/jufajardini/suricata/runs/7733918797?check_suite_focus=true

@jufajardini do you need help on this ?
I did not look your code, but I would ask myself where the macro should be defined so that it makes sense...

[jufajardini]

Now I wonder what share of current PRs will fail CI because of this new check ;-)

:D: Failing because it doesn't recognize a macro called inside a macro, here: https://github.com/jufajardini/suricata/runs/7733918797?check_suite_focus=true

@jufajardini do you need help on this ? I did not look your code, but I would ask myself where the macro should be defined so that it makes sense...

It's fine now, but thanks for asking!

We changed the approached and decided to call the macro in a different place, so the faulty behavior didn't show up.

I did add a ticket with them describing the issue, so they can keep track of it, at least...