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

rust/ike: convert parser to nom7 functions and upgrade dependency #6963

Closed

Conversation

chifflier
Copy link
Contributor

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

Describe changes:

@victorjulien
Copy link
Member

@satta @fhonza can you review this?

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #6963 (3dfd109) into master (786cf41) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6963      +/-   ##
==========================================
- Coverage   77.71%   77.68%   -0.04%     
==========================================
  Files         628      628              
  Lines      185653   185656       +3     
==========================================
- Hits       144289   144232      -57     
- Misses      41364    41424      +60     
Flag Coverage Δ
fuzzcorpus 58.23% <ø> (-0.29%) ⬇️
suricata-verify 54.43% <ø> (+0.07%) ⬆️
unittests 63.03% <ø> (-0.01%) ⬇️

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

reserved,
payload_length
},
data: if let Some(_data) = payload_data { _data } else { b"" }
Copy link
Contributor

Choose a reason for hiding this comment

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

One could use unwrap_or_default() here as well, right?

Copy link
Member

Choose a reason for hiding this comment

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

@chifflier do you want to address this or shall we stage for merge as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed equivalent to unwrap_or_default(). I'd prefer to use the later (shorter and more idiomatic).
Do you want me to push a second commit on this PR, or resubmit?

Copy link
Member

Choose a reason for hiding this comment

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

can you do a new PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! PR sent in #6969

@satta
Copy link
Contributor

satta commented Feb 9, 2022

Also looks good and passes suricata-verify tests for IKE. Also learned about unwrap_or_default() and that take_bits! is apparently gone now :)

@victorjulien
Copy link
Member

replaced by #6969

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants