pcap: refactor delete-when-done to support non-alerts #13906
Conversation
Refactor pcap file deletion to use a single delete-when-done option with three values instead of separate boolean options: - false (default): No deletion - true: Always delete files - "non-alerts": Delete only files with no alerts Key changes: - Replace should_delete/delete_non_alerts_only bools with enum - Move alert counter from global to per-file PcapFileFileVars - Relocate alert counting from PacketAlertFinalize to pcap module - Ensure thread safety for both single and continuous pcap modes - Add comprehensive unit tests for configuration parsing The --pcap-file-delete command line option overrides YAML config and forces "always delete" mode for backward compatibility. Documentation updated to reflect new three-value configuration. Fixes OISF#7786
oferda4
requested review from
jufajardini,
victorjulien and
a team
as code owners
|
NOTE: This PR may contain new authors. |
| * only or pseudo-packet alerts. */ | ||
| if (p->alerts.cnt > 0) { | ||
| /* Best-effort attribute to current pcap file if not already set */ | ||
| if (p->pcap_v.pfv == NULL) { |
There was a problem hiding this comment.
this is invalid - pcap_v is part of a union so the value of pfv is undefined or invalid when other capture methods are in use
There was a problem hiding this comment.
this part of the code should not be aware of any capture method
perhaps we need a callback mechanism of sorts to avoid calling the pcap specific code here
There was a problem hiding this comment.
can this not just move into PcapFileFinalizePacket?
| p->payload = NULL; | ||
| p->payload_len = 0; | ||
|
|
||
| /* In pcap-file mode, associate pseudo end-of-flow packets with the |
There was a problem hiding this comment.
here too we need to keep pcap specific logic out of generic code
| } | ||
| PKT_SET_SRC(np, PKT_SRC_STREAM_TCP_DETECTLOG_FLUSH); | ||
|
|
||
| /* In pcap-file mode, propagate the per-file context so that alerts |
There was a problem hiding this comment.
stream engine should not be aware of the capture method here
| if (p != NULL) { | ||
| p->flags |= PKT_PSEUDO_STREAM_END; | ||
| PKT_SET_SRC(p, PKT_SRC_CAPTURE_TIMEOUT); | ||
| /* In pcap-file mode, attach current pfv so alerts can be counted */ |
There was a problem hiding this comment.
should not be aware of the capture method
|
Thank you for the review! I will try a different approach that won't get capture specific knowledge to the generic code |
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/issues/7786?next_issue_id=7785
Describe changes:
Allowing change the behaviour of --pcap-file-delete to only delete pcaps with no alerts via config.
Previous PR: #13896
Changes:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_BRANCH=OISF/suricata-verify#2664