phd: check exit status of commands, have tests expect success/failure

phd-tests/framework/src/test_vm/mod.rs

@@ -142,6 +142,51 @@ enum VmState {
     Ensured { serial: SerialConsole },
 }
 
+/// Both the output and status of a command.
+pub struct ShellOutput {
+    status: u16,
+    output: String,
+}
+
+impl ShellOutput {
+    /// Consume this [`ShellOutput`], returning the command's output as text
+    /// if the command completed successfully, or an error if it did not.
+    pub fn expect_ok(self) -> Result<String> {
+        if self.status == 0 {
+            Ok(self.output)
+        } else {
+            Err(anyhow!("command exited with non-zero status: {}", self.status))
+        }
+    }
+
+    /// Consume this [`ShellOutput`], returning the command's output as text
+    /// if the command exited with non-zero status, or an error if it exited
+    /// with status zero.
+    pub fn expect_err(self) -> Result<String> {
+        if self.status != 0 {
+            Ok(self.output)
+        } else {
+            Err(anyhow!("command unexpectedly succeeded"))
+        }
+    }
+
+    pub fn ignore_status(self) -> String {
+        self.output
+    }
+
+    pub fn status(&self) -> u16 {
+        self.status
+    }
+
+    /// Get the textual output of the command. You probably should use
+    /// [`ShellOutput::expect_ok`] or [`ShellOutput::expect_err`] to ensure
+    /// the command was processed as expected before asserting on its
+    /// output.
+    pub fn output(&self) -> &str {
+        self.output.as_str()
+    }
+}
+
 /// A virtual machine running in a Propolis server. Test cases create these VMs
 /// using the `factory::VmFactory` embedded in their test contexts.
 ///
@@ -868,7 +913,21 @@ impl TestVm {
     /// waits for another shell prompt to appear using
     /// [`Self::wait_for_serial_output`] and returns any text that was buffered
     /// to the serial console after the command was sent.
-    pub async fn run_shell_command(&self, cmd: &str) -> Result<String> {
+    ///
+    /// After running the shell command, sends `echo $?` to query and return the
+    /// command's return status as well.
+    // TO REVIEWERS: it would be really nice to write this as a function
+    // returning a `struct ShellCommandExecutor` that impls
+    // `Future<Output=String>` where the underlying ShellOutput is automatically
+    // `.expect_ok()`'d. In such a case it would be possible for the struct to
+    // have an `.expect_err()` that replaces teh default `.expect_ok()`
+    // behavior, so that the likely case doesn't need any change in PHD tests.
+    //
+    // unfortunately I don't know how to plumb the futures for that, since we'd
+    // have to close over `&self`, so doing any Boxing to hold an
+    // `async move {}` immediately causes issues.
+    #[must_use]
+    pub async fn run_shell_command(&self, cmd: &str) -> Result<ShellOutput> {
         // Allow the guest OS to transform the input command into a
         // guest-specific command sequence. This accounts for the guest's shell
         // type (which affects e.g. affects how it displays multi-line commands)
@@ -881,7 +940,18 @@ impl TestVm {
         // before actually issuing the final '\n' that issues the command.
         // This ensures that the buffer contents returned by this call contain
         // only the command's output.
-        let out = self
+        let output = self
+            .wait_for_serial_output(
+                self.guest_os.get_shell_prompt(),
+                Duration::from_secs(300),
+            )
+            .await?;
+
+        let status_command_sequence =
+            self.guest_os.shell_command_sequence("echo $?");
+        self.run_command_sequence(status_command_sequence).await?;
+
+        let status = self
             .wait_for_serial_output(
                 self.guest_os.get_shell_prompt(),
                 Duration::from_secs(300),
@@ -891,7 +961,10 @@ impl TestVm {
         // Trim any leading newlines inserted when the command was issued and
         // any trailing whitespace that isn't actually part of the command
         // output. Any other embedded whitespace is the caller's problem.
-        Ok(out.trim().to_string())
+        let output = output.trim().to_string();
+        let status = status.trim().parse::<u16>()?;
+
+        Ok(ShellOutput { status, output })
     }
 
     pub async fn graceful_reboot(&self) -> Result<()> {

phd-tests/tests/src/boot_order.rs

@@ -5,7 +5,7 @@
 use anyhow::{bail, Error};
 use phd_framework::{
     disk::{fat::FatFilesystem, DiskSource},
-    test_vm::{DiskBackend, DiskInterface},
+    test_vm::{DiskBackend, DiskInterface, ShellOutput},
 };
 use phd_testcase::*;
 use std::io::Cursor;
@@ -23,7 +23,7 @@ use efi_utils::{
 pub(crate) async fn run_long_command(
     vm: &phd_framework::TestVm,
     cmd: &str,
-) -> Result<String, Error> {
+) -> Result<ShellOutput, Error> {
     // Ok, this is a bit whacky: something about the line wrapping for long
     // commands causes `run_shell_command` to hang instead of ever actually
     // seeing a response prompt.
@@ -284,7 +284,8 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) {
 
     // If the guest doesn't have an EFI partition then there's no way for boot
     // order preferences to be persisted.
-    let mountline = vm.run_shell_command("mount | grep efivarfs").await?;
+    let mountline =
+        vm.run_shell_command("mount | grep efivarfs").await?.ignore_status();
 
     if !mountline.starts_with("efivarfs on ") {
         warn!(
@@ -298,6 +299,7 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) {
     // reboot, and make sure they're all as we set them.
     if !run_long_command(&vm, &format!("ls {}", efipath(&bootvar(0xffff))))
         .await?
+        .expect_err()?
         .is_empty()
     {
         warn!(

phd-tests/tests/src/boot_order/efi_utils.rs

@@ -326,7 +326,7 @@ pub(crate) async fn read_efivar(
         efipath(varname)
     );
 
-    let hex = run_long_command(vm, &cmd).await?;
+    let hex = run_long_command(vm, &cmd).await?.expect_ok()?;
 
     Ok(unhex(&hex))
 }
@@ -345,7 +345,8 @@ pub(crate) async fn write_efivar(
         efipath(varname)
     );
 
-    let attr_read_bytes = run_long_command(vm, &attr_cmd).await?;
+    let attr_read_bytes =
+        run_long_command(vm, &attr_cmd).await?.ignore_status();
     let attrs = if attr_read_bytes.ends_with(": No such file or directory") {
         // Default attributes if the variable does not exist yet. We expect it
         // to be non-volatile because we are writing it, we expect it to be
@@ -390,7 +391,7 @@ pub(crate) async fn write_efivar(
         efipath(varname)
     );
 
-    let res = run_long_command(vm, &cmd).await?;
+    let res = run_long_command(vm, &cmd).await?.expect_ok()?;
     // If something went sideways and the write failed with something like
     // `invalid argument`...
     if !res.is_empty() {

phd-tests/tests/src/cpuid.rs

@@ -169,7 +169,8 @@ async fn cpuid_boot_test(ctx: &Framework) {
     vm.launch().await?;
     vm.wait_to_boot().await?;
 
-    let cpuinfo = vm.run_shell_command("cat /proc/cpuinfo").await?;
+    let cpuinfo =
+        vm.run_shell_command("cat /proc/cpuinfo").await?.expect_ok()?;
     info!(cpuinfo, "/proc/cpuinfo output");
     assert!(cpuinfo.contains(
         std::str::from_utf8(BRAND_STRING).unwrap().trim_matches('\0')

phd-tests/tests/src/crucible/migrate.rs

@@ -19,10 +19,13 @@ async fn smoke_test(ctx: &Framework) {
     source.launch().await?;
     source.wait_to_boot().await?;
 
-    let lsout = source.run_shell_command("ls foo.bar 2> /dev/null").await?;
+    let lsout = source
+        .run_shell_command("ls foo.bar 2> /dev/null")
+        .await?
+        .expect_err()?;
     assert_eq!(lsout, "");
-    source.run_shell_command("touch ./foo.bar").await?;
-    source.run_shell_command("sync ./foo.bar").await?;
+    source.run_shell_command("touch ./foo.bar").await?.expect_ok()?;
+    source.run_shell_command("sync ./foo.bar").await?.expect_ok()?;
 
     disk.set_generation(2);
     let mut target = ctx
@@ -32,7 +35,7 @@ async fn smoke_test(ctx: &Framework) {
     target
         .migrate_from(&source, Uuid::new_v4(), MigrationTimeout::default())
         .await?;
-    let lsout = target.run_shell_command("ls foo.bar").await?;
+    let lsout = target.run_shell_command("ls foo.bar").await?.expect_ok()?;
     assert_eq!(lsout, "foo.bar");
 }
 
@@ -63,27 +66,34 @@ async fn load_test(ctx: &Framework) {
             )
             .as_str(),
         )
-        .await?;
+        .await?
+        .expect_ok()?;
     assert!(ddout.contains(format!("{}+0 records in", block_count).as_str()));
 
     // Compute the data's hash.
-    let sha256sum_out = source.run_shell_command("sha256sum rand.txt").await?;
+    let sha256sum_out =
+        source.run_shell_command("sha256sum rand.txt").await?.expect_ok()?;
     let checksum = sha256sum_out.split_whitespace().next().unwrap();
     info!("Generated SHA256 checksum: {}", checksum);
 
     // Start copying the generated file into a second file, then start a
     // migration while that copy is in progress.
-    source.run_shell_command("dd if=./rand.txt of=./rand_new.txt &").await?;
+    source
+        .run_shell_command("dd if=./rand.txt of=./rand_new.txt &")
+        .await?
+        .expect_ok()?;
     target
         .migrate_from(&source, Uuid::new_v4(), MigrationTimeout::default())
         .await?;
 
     // Wait for the background command to finish running, then compute the
     // hash of the copied file. If all went well this will match the hash of
     // the source file.
-    target.run_shell_command("wait $!").await?;
-    let sha256sum_target =
-        target.run_shell_command("sha256sum rand_new.txt").await?;
+    target.run_shell_command("wait $!").await?.expect_ok()?;
+    let sha256sum_target = target
+        .run_shell_command("sha256sum rand_new.txt")
+        .await?
+        .expect_ok()?;
     let checksum_target = sha256sum_target.split_whitespace().next().unwrap();
     assert_eq!(checksum, checksum_target);
 }

phd-tests/tests/src/crucible/smoke.rs

@@ -37,6 +37,7 @@ async fn guest_reboot_test(ctx: &Framework) {
     vm.launch().await?;
     vm.wait_to_boot().await?;
 
+    // XXX: use graceful_reboot() now.
     // Don't use `run_shell_command` because the guest won't echo another prompt
     // after this.
     vm.send_serial_str("reboot\n").await?;
@@ -64,10 +65,11 @@ async fn shutdown_persistence_test(ctx: &Framework) {
 
     // Verify that the test file doesn't exist yet, then touch it, flush it, and
     // shut down the VM.
-    let lsout = vm.run_shell_command("ls foo.bar 2> /dev/null").await?;
+    let lsout =
+        vm.run_shell_command("ls foo.bar 2> /dev/null").await?.expect_err()?;
     assert_eq!(lsout, "");
-    vm.run_shell_command("touch ./foo.bar").await?;
-    vm.run_shell_command("sync ./foo.bar").await?;
+    vm.run_shell_command("touch ./foo.bar").await?.expect_ok()?;
+    vm.run_shell_command("sync ./foo.bar").await?.expect_ok()?;
     vm.stop().await?;
     vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(60))
         .await?;
@@ -82,6 +84,7 @@ async fn shutdown_persistence_test(ctx: &Framework) {
     vm.wait_to_boot().await?;
 
     // The touched file from the previous VM should be present in the new one.
-    let lsout = vm.run_shell_command("ls foo.bar 2> /dev/null").await?;
+    let lsout =
+        vm.run_shell_command("ls foo.bar 2> /dev/null").await?.expect_ok()?;
     assert_eq!(lsout, "foo.bar");
 }

phd-tests/tests/src/disk.rs

@@ -37,9 +37,12 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) {
     // try to check that the disk is located there and fail the test early if
     // it's not. If the by-path directory is missing, put up a warning and hope
     // for the best.
-    let dev_disk = vm.run_shell_command("ls /dev/disk").await?;
+    let dev_disk = vm.run_shell_command("ls /dev/disk").await?.ignore_status();
     if dev_disk.contains("by-path") {
-        let ls = vm.run_shell_command("ls -la /dev/disk/by-path").await?;
+        let ls = vm
+            .run_shell_command("ls -la /dev/disk/by-path")
+            .await?
+            .expect_ok()?;
         info!(%ls, "guest disk device paths");
         assert!(ls.contains("virtio-pci-0000:00:18.0 -> ../../vda"));
     } else {
@@ -49,17 +52,19 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) {
         );
     }
 
-    vm.run_shell_command("mkdir /phd").await?;
+    vm.run_shell_command("mkdir /phd").await?.expect_ok()?;
 
     // The disk is read-only, so pass the `ro` option to `mount` so that it
     // doesn't complain about not being able to mount for writing.
-    let mount = vm.run_shell_command("mount -o ro /dev/vda /phd").await?;
+    let mount =
+        vm.run_shell_command("mount -o ro /dev/vda /phd").await?.expect_ok()?;
     assert_eq!(mount, "");
 
     // The file should be there and have the expected contents.
-    let ls = vm.run_shell_command("ls /phd").await?;
+    let ls = vm.run_shell_command("ls /phd").await?.expect_ok()?;
     assert_eq!(ls, "hello_oxide.txt");
 
-    let cat = vm.run_shell_command("cat /phd/hello_oxide.txt").await?;
+    let cat =
+        vm.run_shell_command("cat /phd/hello_oxide.txt").await?.expect_ok()?;
     assert_eq!(cat, HELLO_MSG);
 }

phd-tests/tests/src/framework.rs

@@ -13,6 +13,7 @@ async fn multiline_serial_test(ctx: &Framework) {
     vm.launch().await?;
     vm.wait_to_boot().await?;
 
-    let out = vm.run_shell_command("echo \\\nhello \\\nworld").await?;
+    let out =
+        vm.run_shell_command("echo \\\nhello \\\nworld").await?.expect_ok()?;
     assert_eq!(out, "hello world");
 }

phd-tests/tests/src/hw.rs

@@ -17,15 +17,23 @@ async fn lspci_lifecycle_test(ctx: &Framework) {
     vm.launch().await?;
     vm.wait_to_boot().await?;
 
-    let lspci = vm.run_shell_command(LSPCI).await?;
-    let lshw = vm.run_shell_command(LSHW).await?;
+    // XXX: do not `ignore_status()` on these commands! They fail for any number
+    // of reasons on different guests:
+    // * sudo may not exist (some Alpine)
+    // * lshw may not exist (Debian)
+    // * we may not input a sudo password (Ubuntu)
+
+    let lspci = vm.run_shell_command(LSPCI).await?.ignore_status();
+    let lshw = vm.run_shell_command(LSHW).await?.ignore_status();
     ctx.lifecycle_test(vm, &[Action::StopAndStart], move |vm| {
         let lspci = lspci.clone();
         let lshw = lshw.clone();
         Box::pin(async move {
-            let new_lspci = vm.run_shell_command(LSPCI).await.unwrap();
+            let new_lspci =
+                vm.run_shell_command(LSPCI).await.unwrap().ignore_status();
             assert_eq!(new_lspci, lspci);
-            let new_lshw = vm.run_shell_command(LSHW).await.unwrap();
+            let new_lshw =
+                vm.run_shell_command(LSHW).await.unwrap().ignore_status();
             assert_eq!(new_lshw, lshw);
         })
     })