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

Backport: endace: Fix source-dag timestamps #10148

Closed
wants to merge 2 commits into from
Closed

Conversation

sfd
Copy link
Contributor

@sfd sfd commented Jan 9, 2024

Bug: #6618

Fix Endace ERF to SCTime_t timestamp conversion

Fix typo preventing compilation with --enable-dag

(cherry picked from commit 879db3d)

Fix compiler const warning with --enable-dag

Link to redmine ticket: 6620

Updates PR #10092

@sfd sfd requested a review from victorjulien as a code owner January 9, 2024 23:31
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f2e83e4) 82.42% compared to head (53e6f51) 82.35%.

Additional details and impacted files
@@              Coverage Diff               @@
##           main-7.0.x   #10148      +/-   ##
==============================================
- Coverage       82.42%   82.35%   -0.08%     
==============================================
  Files             970      970              
  Lines          274011   274007       -4     
==============================================
- Hits           225858   225659     -199     
- Misses          48153    48348     +195     
Flag Coverage Δ
fuzzcorpus 64.09% <ø> (-0.26%) ⬇️
suricata-verify 61.09% <ø> (-0.03%) ⬇️
unittests 62.92% <ø> (+<0.01%) ⬆️

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

@sfd
Copy link
Contributor Author

sfd commented Jan 10, 2024

Not sure what is up with the Rust check.The Debian 12 build might need to be re-run.

@sfd
Copy link
Contributor Author

sfd commented Jan 14, 2024

@victorjulien not sure what is wrong with the Rust test? The debian build probably just needs to be rerun.

@catenacyber
Copy link
Contributor

catenacyber commented Jan 15, 2024

not sure what is wrong with the Rust test? The debian build probably just needs to be rerun.

You should rebase to latest main-7.0.x, and it should go away

Bug: OISF#6618.

Fix Endace ERF to SCTime_t timestamp conversion

Fix typo preventing compilation with --enable-dag

(cherry picked from commit 879db3d)
@catenacyber
Copy link
Contributor

"Bug: 6620" is not a good commit title...
And there is no cherry-pick line for this one... Do you know why ?

Bug: 6620.

Fix const warning with --enable=dag

Fix prototype clang-format errors

Master change in OISF#10149
@sfd
Copy link
Contributor Author

sfd commented Jan 16, 2024

"Bug: 6620" is not a good commit title... And there is no cherry-pick line for this one... Do you know why ?

Updated the commit title.

It's not a cherry pick since the equivalent master PR is pending, see PR 10149. Could wait for that to be approved and cherry pick instead, but is equivalent? The compiler warning was noticed and fixed in the backport PR.

@catenacyber
Copy link
Contributor

"Bug: 6620" is not a good commit title... And there is no cherry-pick line for this one... Do you know why ?

Updated the commit title.

It's not a cherry pick since the equivalent master PR is pending, see PR 10149. Could wait for that to be approved and cherry pick instead, but is equivalent? The compiler warning was noticed and fixed in the backport PR.

I think we want to merge first in master, and then cherry pick, so this PR is waiting on 10149.
The rationale is that even if code is equivalent, we gets a better git history.

@sfd
Copy link
Contributor Author

sfd commented Jan 17, 2024

Can do it that way if you prefer.
The alternative would be to back out the compiler warning commit and merge this PR, thereby bringing main-7.0.x up to date with master, then merge 10149 to master, and finally backport that as a separate PR?
This PR is more important since it fixes actual bugs in 7.x as opposed to just compiler warnings.

@catenacyber
Copy link
Contributor

@sfd thanks for your work.

I am sorry I am not an expert on Endace... I just know a little about the GitHub workflow for Suricata, and why the CI was red for rust... I guess we will have to wait for someone with the endace knowledge to do the approvals and merge in the order they see fit...

@victorjulien
Copy link
Member

@sfd thanks for your work.

I am sorry I am not an expert on Endace... I just know a little about the GitHub workflow for Suricata, and why the CI was red for rust... I guess we will have to wait for someone with the endace knowledge to do the approvals and merge in the order they see fit...

@catenacyber @sfd is the Endace expert as he represents Endace here. We can trust that his Endace specific logic is correct, but can still review on style and process.

@catenacyber
Copy link
Contributor

So, now that #10149 was merged, could you make a new PR with the first commit of this one + a cherry-pick of the one from 10149 merged in master ? How does that sound to you ?

@sfd
Copy link
Contributor Author

sfd commented Jan 22, 2024

Closed ; continued in PR #10216

@sfd sfd closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants