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

Dns move keywords to rust 3195 v1.1 #12483

Closed

Conversation

catenacyber
Copy link
Contributor

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/3195 (generic)

Describe changes:

  • dns: move keywords to rust

#11932 next protocol

Draft : still need to craft SV tests ro replace unit tests DetectDnsQueryTest

@@ -739,6 +727,14 @@ void SigTableSetup(void)
ScDetectSipRegister();
ScDetectTemplateRegister();
ScDetectLdapRegister();
SCDetectDNSRegister();
/* For lua : register these generic engines from here for now */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasonish what is going on here for lua ?

Copy link
Member

Choose a reason for hiding this comment

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

? I'm not really familiar with this bit. Are these registrations for Lua only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently cf git grep '"dns_request"' it gets used in detect-lua.c and it is needed for the SV test about dns and lua

Copy link
Member

Choose a reason for hiding this comment

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

I think we're looking to cleanup all this Lua stuff related to this. I admit I'm not that familiar with how it works at this time.

@jasonish
Copy link
Member

Should probably have its own ticket, and be related to this roadmap entry: https://redmine.openinfosecfoundation.org/issues/7140

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

Attention: Patch coverage is 94.89247% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.54%. Comparing base (d63ad75) to head (ad1b4a3).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12483      +/-   ##
==========================================
+ Coverage   80.52%   80.54%   +0.01%     
==========================================
  Files         923      917       -6     
  Lines      259176   258714     -462     
==========================================
- Hits       208708   208375     -333     
+ Misses      50468    50339     -129     
Flag Coverage Δ
fuzzcorpus 56.16% <61.42%> (+0.10%) ⬆️
livemode 19.40% <35.60%> (-0.01%) ⬇️
pcap 44.22% <54.30%> (+0.03%) ⬆️
suricata-verify 63.35% <92.87%> (+0.02%) ⬆️
unittests 58.40% <47.31%> (-0.06%) ⬇️

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

@jasonish
Copy link
Member

I think we should resolve #11647 before doing this - which I plan on getting to. That and DNS parity are next on my 8 priority list after wrapping up some Lua stuff this week or next.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24366

@catenacyber
Copy link
Contributor Author

Should probably have its own ticket, and be related to this roadmap entry: https://redmine.openinfosecfoundation.org/issues/7140

This is not a protocol conversion, just detect keywords moving wrapper code from c to rust...

@catenacyber
Copy link
Contributor Author

I think we should resolve #11647 before doing this - which I plan on getting to. That and DNS parity are next on my 8 priority list after wrapping up some Lua stuff this week or next.

This is kind of independent of #11647 which is adding a new keyword, or am I missing something?

@jasonish
Copy link
Member

I think we should resolve #11647 before doing this - which I plan on getting to. That and DNS parity are next on my 8 priority list after wrapping up some Lua stuff this week or next.

This is kind of independent of #11647 which is adding a new keyword, or am I missing something?

Maybe it won't create conflicts. But adding new keywords in C, and converting keywords to Rust do sound conflicting :)

@jasonish
Copy link
Member

I think we should resolve #11647 before doing this - which I plan on getting to. That and DNS parity are next on my 8 priority list after wrapping up some Lua stuff this week or next.

This is kind of independent of #11647 which is adding a new keyword, or am I missing something?

And it does conflict with some WIP I have on https://redmine.openinfosecfoundation.org/issues/5642.

@catenacyber
Copy link
Contributor Author

But adding new keywords in C, and converting keywords to Rust do sound conflicting :)

I see how it sounds, but it is not conflicting in essence and I can do the new keywords conversion later

And it does conflict with some WIP I have on https://redmine.openinfosecfoundation.org/issues/5642.

Does this help you ?
I feel rust way is faster to code such keywords...

@catenacyber
Copy link
Contributor Author

Next in #12496

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.

3 participants