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

packet_analysis: Introduce PacketAnalyzer::__disable_analyzer() #2443

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

awelzel
Copy link
Contributor

@awelzel awelzel commented Sep 29, 2022

@rsmmr - mind skimming if this goes into a direction that you think is okay for #2399? It exploits the fact that the manager is holding AnalyzerPtr's for the individual analyzers.

Will hookup Analyzer::disable_analyzer() and add a test or two, but in case something stands out better to know early.

@rsmmr
Copy link
Member

rsmmr commented Sep 29, 2022

Yes, that looks good to me. (I keep forgetting that packet analyzers are instantiated only once, so yes, putting the state right in the analyzer makes sense).

@awelzel awelzel force-pushed the topic/awelzel/2399-packet-analyzer-disabling branch from 98c5778 to fea3246 Compare September 29, 2022 14:38
@awelzel
Copy link
Contributor Author

awelzel commented Sep 29, 2022

Yes, that looks good to me. (I keep forgetting that packet analyzers are instantiated only once, so yes, putting the state right in the analyzer makes sense).

I'm marking this ready. I'm uncertain about the Analyzer::disable_analyzer() extension: It seems nice to have a single entry point and be able to reuse the Analyzer::disabled_analyzers mechanism, but could also see it being separated/duplicated in the PacketAnalyzer module over at scripts/base/packet-protocols/main.zeek to not mix up the different analyzers.

@rsmmr - mainly looking for opinion/guidance here. Over in the issue you mentioned extending the existing one and the current state is my interpretation of that :-)

And I think we could then also extend the script-side disable_analyzer to support packet analyzers as well.

@awelzel awelzel marked this pull request as ready for review September 29, 2022 14:48
Copy link
Member

@rsmmr rsmmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it this way. So ready to merge?

And ... what about file analyzers? Want to extend disable_analyzer to that next?

This adds machinery to the packet_analysis manager for disabling
and enabling packet analyzers and implements two low-level bifs
to use it.

Extend Analyzer::enable_analyzer() and Analyzer::disable_analyzer()
to transparently work with packet analyzers, too. This also allows
to add packet analyzers to Analyzer::disabled_analyzers.
With packet analyzers being toggle-able at runtime these can go.
They hadn't been consistently implemented either (VXLAN, Geneve).
@awelzel awelzel force-pushed the topic/awelzel/2399-packet-analyzer-disabling branch from 5358b30 to 3e0374f Compare September 30, 2022 07:32
@rsmmr rsmmr merged commit 4c788f1 into master Sep 30, 2022
@rsmmr rsmmr deleted the topic/awelzel/2399-packet-analyzer-disabling branch September 30, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants