Edward O'Callaghan submitted this change.

View Change

CHROMIUM: avl_tool: more gracefully handle termination by SIGINT

Since interrupting the test process may be dangerous (leaving the flash
in an inconsistent state), we'll catch SIGINT and print a warning the
first time, also using it as a signal that we should stop at a
convenient time. Any following SIGINT will be handled as normal (killing
the process).

BUG=b:143251344
TEST=Run tool and verify it exits after a test with a single ^C, exits
     immediately given two.
BRANCH=None

Original-Cq-Depend: chromium:2059548
Original-Change-Id: Ib8a7799cba6dbca57dc7f1d3c87521f132c21818
Original-Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+/2050050
Original-Tested-by: Edward O'Callaghan <quasisec@chromium.org>
Original-Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>

Change-Id: If43aea0580fcc7e698daad2ffe085a3c9da5bc41
Signed-off-by: Edward O'Callaghan <quasisec@google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/49915
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
---
M util/flashrom_tester/Cargo.toml
M util/flashrom_tester/src/main.rs
M util/flashrom_tester/src/tester.rs
M util/flashrom_tester/src/tests.rs
4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/util/flashrom_tester/Cargo.toml b/util/flashrom_tester/Cargo.toml
index 50f2c4a..0898d3c 100644
--- a/util/flashrom_tester/Cargo.toml
+++ b/util/flashrom_tester/Cargo.toml
@@ -19,6 +19,7 @@
 clap = { version = "2.33", default-features = false, optional = true }
 flashrom = { path = "flashrom/" }
 log = { version = "0.4", features = ["std"] }
+nix = "0.14.1"
 rand = "0.6.4"
 serde_json = "1"
 sys-info = "0.5.7"
diff --git a/util/flashrom_tester/src/main.rs b/util/flashrom_tester/src/main.rs
index 1cc525e..e589ee1 100644
--- a/util/flashrom_tester/src/main.rs
+++ b/util/flashrom_tester/src/main.rs
@@ -42,6 +42,7 @@
 use flashrom::FlashChip;
 use flashrom_tester::{tester, tests};
 use std::path::PathBuf;
+use std::sync::atomic::AtomicBool;
 
 pub mod built_info {
     include!(concat!(env!("OUT_DIR"), "/built.rs"));
@@ -136,8 +137,45 @@
         print_layout,
         output_format,
         test_names,
+        Some(handle_sigint()),
     ) {
         eprintln!("Failed to run tests: {:?}", e);
         std::process::exit(1);
     }
 }
+
+/// Catch exactly one SIGINT, printing a message in response and setting a flag.
+///
+/// The returned value is false by default, becoming true after a SIGINT is
+/// trapped.
+///
+/// Once a signal is trapped, the default behavior is restored (terminating
+/// the process) for future signals.
+fn handle_sigint() -> &'static AtomicBool {
+    use nix::libc::c_int;
+    use nix::sys::signal::{self, SigHandler, Signal};
+    use std::sync::atomic::Ordering;
+
+    unsafe {
+        let _ = signal::signal(Signal::SIGINT, SigHandler::Handler(sigint_handler));
+    }
+    static TERMINATE_FLAG: AtomicBool = AtomicBool::new(false);
+
+    extern "C" fn sigint_handler(_: c_int) {
+        const STDERR_FILENO: c_int = 2;
+        static MESSAGE: &[u8] = b"
+WARNING: terminating tests prematurely may leave Flash in an inconsistent state,
+rendering your machine unbootable. Testing will end on completion of the current
+test, or press ^C again to exit immediately (possibly bricking your machine).
+";
+
+        // Use raw write() because signal-safety is a very hard problem
+        let _ = nix::unistd::write(STDERR_FILENO, MESSAGE);
+        unsafe {
+            let _ = signal::signal(Signal::SIGINT, SigHandler::SigDfl);
+        }
+        TERMINATE_FLAG.store(true, Ordering::Release);
+    }
+
+    &TERMINATE_FLAG
+}
diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs
index fbef201..3150a43 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -39,6 +39,7 @@
 use flashrom::{FlashChip, Flashrom, FlashromCmd};
 use serde_json::json;
 use std::mem::MaybeUninit;
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Mutex;
 
 // type-signature comes from the return type of lib.rs workers.
@@ -480,6 +481,7 @@
     chip: FlashChip,
     cmd: &FlashromCmd,
     ts: TS,
+    terminate_flag: Option<&AtomicBool>,
 ) -> Vec<(String, (TestConclusion, Option<TestError>))>
 where
     T: TestCase + Copy,
@@ -489,6 +491,13 @@
 
     let mut results = Vec::new();
     for t in ts {
+        if terminate_flag
+            .map(|b| b.load(Ordering::Acquire))
+            .unwrap_or(false)
+        {
+            break;
+        }
+
         let result = decode_test_result(env.run_test(t), t.expected_result());
         results.push((t.get_name().into(), result));
     }
diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs
index dd75689..9ef98e5 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -40,6 +40,7 @@
 use std::collections::{HashMap, HashSet};
 use std::fs::File;
 use std::io::{BufRead, Write};
+use std::sync::atomic::AtomicBool;
 
 const LAYOUT_FILE: &'static str = "/tmp/layout.file";
 
@@ -82,6 +83,7 @@
     print_layout: bool,
     output_format: OutputFormat,
     test_names: Option<TN>,
+    terminate_flag: Option<&AtomicBool>,
 ) -> Result<(), Box<dyn std::error::Error>> {
     let p = path.to_string();
     let cmd = FlashromCmd { path: p, fc };
@@ -142,7 +144,7 @@
 
     // ------------------------.
     // Run all the tests and collate the findings:
-    let results = tester::run_all_tests(fc, &cmd, tests);
+    let results = tester::run_all_tests(fc, &cmd, tests, terminate_flag);
 
     // Any leftover filtered names were specified to be run but don't exist
     for leftover in filter_names.iter().flatten() {

To view, visit change 49915. To unsubscribe, or for help writing mail filters, visit settings.