Bug/12187 v6 reliably detect Suricata startup

[KEIAHNY]

PR Type

Bug fix, Enhancement

---

Description

• Wait for Suricata startup via pidfile and socket

• Replace process scanning with pidfile-based PID detection

• Poll suricatasc uptime to confirm Suricata readiness

• Add socket config option and -s CLI flag

---

Diagram Walkthrough

flowchart LR
  A["Delete stale pidfile"] -- "launch with --pidfile" --> B["Start Suricata"]
  B -- "read & verify /proc comm" --> C["Get Suricata PID"]
  C -- "poll suricatasc uptime" --> D["Suricata ready"]
  C -- "timeout or early exit" --> E["Kill Suricata and panic"]

Loading

File Walkthrough

	Relevant files
Enhancement	argument.rs Add CLI option for Suricata socket path                                    src/argument.rs Added -s/--path_to_socket CLI option for Suricata command +4/-0      yaml.rs Add socket path to Suriconf configuration                                src/yaml.rs Added socket field to Suriconf struct Parse socket path from CLI arg or config via find_socket +12/-0
Bug fix	suricata.rs Wait for Suricata startup via pidfile and socket                  src/suricata.rs Added PIDFILE constant and delete_pid_file/set_pid_file helpers Replaced check_process_name_for_suricata_main with pidfile-based get_suricata_pid Added wait_on_suricata_start polling suricatasc uptime via socket Kill Suricata and panic on startup failure; stdout set to null +85/-17
Documentation	README.md Document socket configuration option                                          README.md Documented new socket configuration option +1/-0
Configuration changes	suriconf.yaml Add default socket path to config                                                src/suriconf.yaml Added default socket path /var/run/suricata/suricata-command.socket +1/-0

---

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Race condition In kill_suricata, the newly added check panics if pkill -SIGKILL Suricata-Main returns a non-zero status. However, pkill also returns exit code 1 when no process matched, which happens if Suricata exits on its own in the window between the "still alive" check and the pkill invocation (e.g., graceful termination completing late). In that scenario the tool panics with "Process failed." even though the goal (Suricata stopped) was already achieved. Consider treating a "no processes matched" result as success. if !output.success() { panic!("Process failed."); } Panic on try_wait error Inside wait_on_suricata_start, child.try_wait().expect("Unable to get Suricata status.") uses expect inside a function that returns Result, so an I/O error from try_wait panics instead of being propagated as an Err like the other failure paths in this function. It also bypasses the kill_suricata cleanup that runs on other errors. This should map the error into the returned Err variant. if let Some(status) = child.try_wait().expect("Unable to get Suricata status.") { Config coupling wait_on_suricata_start polls the socket path taken from the suriconf socket setting, but Suricata itself reads its socket path from its own configuration file. If these two values diverge (user overrides one but not the other), Suricata may be fully healthy yet the check fails, blocking for the full 120 iterations before returning "Suricata could not start." and killing a healthy process. Not certain this is wrong for the intended workflow, but worth verifying that the socket paths cannot get out of sync, or deriving both from a single source. let socket = socket.to_str().ok_or("Socket path is not valid UTF-8.")?; for _ in 0..120 { let output = Command::new("sudo") .arg("-n") .arg("suricatasc") .arg("-c") .arg("uptime") .arg(socket) .output() .map_err(|e| format!("Unable to execute Suricata socket control tool: {e}"))?; if output.status.success() { return Ok(pid) } if let Some(status) = child.try_wait().expect("Unable to get Suricata status.") { return Err(format!("Suricata exited during startup with status: {status}.")); } thread::sleep(Duration::from_millis(1000)); } Err("Suricata could not start.".into())

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fail fast when Suricata exits during startup If Suricata fails to launch (e.g., bad configuration), this loop blindly waits the full 20 seconds before returning a misleading "Suricata process not found" error. Pass the child handle into the loop and check child.try_wait() each iteration to fail fast with the actual exit status. src/suricata.rs [313-327] -fn get_suricata_pid() -> Result<i32, String> { +fn get_suricata_pid(child: &mut Child) -> Result<i32, String> { for _ in 0..20 { if let Ok(content) = fs::read_to_string(PIDFILE) { if let Ok(pid) = content.trim().parse::<i32>() { if let Ok(comm) = fs::read_to_string(format!("/proc/{}/comm", pid)) { if comm.trim() == "Suricata-Main" { - return Ok(pid); + return Ok(pid); } } } } + if let Some(status) = child.try_wait().expect("Unable to get Suricata status.") { + return Err(format!("Suricata exited during startup with status: {status}.")); + } thread::sleep(Duration::from_millis(1000)); } - Err("Suricata process not found.".into()) + Err("Suricata PID file not found.".into()) } Suggestion importance[1-10]: 7 __ Why: The suggestion is correct: get_suricata_pid runs before the child.try_wait() check in wait_on_suricata_start, so a failed Suricata launch causes a full 20-second blind wait with a misleading error. Checking the child in this loop is a real robustness improvement, and the improved code is consistent with the existing pattern used later in wait_on_suricata_start.	Medium
General	Distinguish PID file read errors from absence The PID file is created by a root-run Suricata process (via sudo), but it is read here without elevated privileges; if the file's permissions restrict read access, the error is silently swallowed and startup detection stalls for the full 20-second timeout. Match the error kind explicitly so that "not found yet" is retried but other errors (e.g., permission denied) surface immediately. src/suricata.rs [315] - if let Ok(content) = fs::read_to_string(PIDFILE) { + match fs::read_to_string(PIDFILE) { + Ok(content) => { /* parse pid ... */ } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { /* retry */ } + Err(e) => return Err(format!("Unable to read Suricata PID file: {e}.")), + } Suggestion importance[1-10]: 5 __ Why: The concern is plausible: the PID file is created by a root-run Suricata process, and a permission error would be silently swallowed and stall the loop. Matching ErrorKind::NotFound to retry while surfacing other errors is a correct error-handling refinement, though it is not a critical issue since the loop would still time out.	Low
	Preserve Suricata startup output for debugging Redirecting stdout to Stdio::null() discards all of Suricata's startup output, making configuration errors and warnings undiagnosable. Use Stdio::inherit() so Suricata's stdout goes to the parent's terminal while stderr is still captured by the reader thread. src/suricata.rs [80-81] let mut child = Command::new("sudo") .arg("-n") .args(args) - .stdout(Stdio::null()) + .stdout(Stdio::inherit()) .stderr(Stdio::piped()) Suggestion importance[1-10]: 4 __ Why: The suggestion is technically valid (using Stdio::inherit() avoids the pipe-buffer deadlock that the PR's change from piped() to null() was likely fixing while keeping output visible), but it contradicts the PR's deliberate intent to silence stdout. It is a reasonable but debatable behavior change with moderate debugging benefit.	Low
	Accept generic path reference for socket The function accepts &PathBuf instead of &Path, forcing callers to have a PathBuf and requiring the extra to_str() conversion. Accept &Path and use socket.to_str() directly, which is the idiomatic Rust API for path parameters. src/suricata.rs [328] -fn wait_on_suricata_start(socket: &PathBuf, child: &mut Child) -> Result<i32, String> { +fn wait_on_suricata_start(socket: &Path, child: &mut Child) -> Result<i32, String> { Suggestion importance[1-10]: 3 __ Why: Using &Path instead of &PathBuf is the idiomatic Rust convention and Path is already imported, but the change is purely stylistic with no functional impact; the existing to_str() call works the same on both types.	Low

• Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.