Merge branch 'ixi/shell-command-output-check-2' into ixi/shell-comman…

…d-output-check

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

@@ -18,7 +18,7 @@ use crate::{
     Framework,
 };
 
-use anyhow::{anyhow, Context, Result};
+use anyhow::{anyhow, bail, Context, Result};
 use camino::Utf8PathBuf;
 use core::result::Result as StdResult;
 use propolis_client::{
@@ -148,72 +148,113 @@ pub struct ShellOutput {
     output: String,
 }
 
-pub struct ShellOutputExecutor {
-    fut: Pin<Box<dyn std::future::Future<Output=Result<ShellOutput>>>>
+/// Description of the acceptable status codes from executing a command in a
+/// [`TestVm::run_shell_command`].
+// This could reasonably have a `Status(u16)` variant to check specific non-zero
+// statuses, but specific codes are not terribly portable! In the few cases we
+// can expect a specific status for errors, those specific codes change between
+// f.ex illumos and Linux guests.
+enum StatusCheck {
+    Ok,
+    NotOk,
 }
 
-pub struct ShellOutputOkExecutor {
-    fut: Pin<Box<dyn std::future::Future<Output=Result<ShellOutput>>>>
+pub struct ShellOutputExecutor<'ctx> {
+    vm: &'ctx TestVm,
+    cmd: &'ctx str,
+    status_check: Option<StatusCheck>,
 }
 
-use std::pin::Pin;
-use std::task::Poll;
-
-impl std::future::Future for ShellOutputExecutor {
-    type Output = Result<ShellOutput>;
-
-    fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll<Self::Output> {
-        self.fut.as_mut().poll(cx)
+impl<'a> ShellOutputExecutor<'a> {
+    pub fn ignore_status(mut self) -> ShellOutputExecutor<'a> {
+        self.status_check = None;
+        self
     }
-}
 
-impl std::future::Future for ShellOutputOkExecutor {
-    type Output = Result<String>;
+    pub fn check_ok(mut self) -> ShellOutputExecutor<'a> {
+        self.status_check = Some(StatusCheck::Ok);
+        self
+    }
 
-    fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll<Self::Output> {
-        match self.fut.as_mut().poll(cx) {
-            Poll::Ready(t) => Poll::Ready(t.and_then(|res| res.expect_ok())),
-            Poll::Pending => Poll::Pending,
-        }
+    pub fn check_err(mut self) -> ShellOutputExecutor<'a> {
+        self.status_check = Some(StatusCheck::NotOk);
+        self
     }
 }
+use futures::FutureExt;
 
-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))
-        }
-    }
+impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> {
+    type Output = Result<String>;
+    type IntoFuture = futures::future::BoxFuture<'a, Result<String>>;
+
+    fn into_future(self) -> Self::IntoFuture {
+        let cmd = Box::pin(async move {
+            // 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) and serial console buffering discipline.
+            let command_sequence =
+                self.vm.guest_os.shell_command_sequence(self.cmd);
+            self.vm.run_command_sequence(command_sequence).await?;
+
+            // `shell_command_sequence` promises that the generated command
+            // sequence clears buffer of everything up to and including the
+            // input command 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 output = self
+                .vm
+                .wait_for_serial_output(
+                    self.vm.guest_os.get_shell_prompt(),
+                    Duration::from_secs(300),
+                )
+                .await?;
 
-    /// 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"))
-        }
-    }
+            let status_command_sequence =
+                self.vm.guest_os.shell_command_sequence("echo $?");
+            self.vm.run_command_sequence(status_command_sequence).await?;
 
-    pub fn ignore_status(self) -> String {
-        self.output
-    }
+            let status = self
+                .vm
+                .wait_for_serial_output(
+                    self.vm.guest_os.get_shell_prompt(),
+                    Duration::from_secs(300),
+                )
+                .await?;
 
-    pub fn status(&self) -> u16 {
-        self.status
-    }
+            // 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.
+            let output = output.trim().to_string();
+            let status = status.trim().parse::<u16>()?;
 
-    /// 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()
+            Ok(ShellOutput { status, output })
+        });
+        cmd.map(move |res| {
+            res.and_then(|out| {
+                match self.status_check {
+                    Some(StatusCheck::Ok) => {
+                        if out.status != 0 {
+                            bail!("expected status 0, got {}", out.status);
+                        }
+                    }
+                    Some(StatusCheck::NotOk) => {
+                        if out.status != 0 {
+                            bail!(
+                                "expected non-zero status, got {}",
+                                out.status
+                            );
+                        }
+                    }
+                    None => {
+                        // No check, always a success regardless of exit status
+                    }
+                }
+                Ok(out.output)
+            })
+        })
+        .boxed()
     }
 }
 
@@ -946,59 +987,19 @@ impl TestVm {
     ///
     /// 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.
-    pub fn run_shell_command<'a>(&'a self, cmd: &'a str) -> impl std::future::Future<Output=Result<ShellOutput>> + 'a {
-        let fut = Box::pin(async move {
-        // 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)
-        // and serial console buffering discipline.
-        let command_sequence = self.guest_os.shell_command_sequence(cmd);
-        self.run_command_sequence(command_sequence).await?;
-
-        // `shell_command_sequence` promises that the generated command sequence
-        // clears buffer of everything up to and including the input command
-        // 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 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),
-            )
-            .await?;
-
-        // 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.
-        let output = output.trim().to_string();
-        let status = status.trim().parse::<u16>()?;
-
-        Ok(ShellOutput { status, output })
-        });
-
+    ///
+    /// This will return an error if the command returns a non-zero status by
+    /// default; to ignore the status or expect a non-zero as a positive
+    /// condition, see [`ShellOutputExecutor::ignore_status`] or
+    /// [`ShellOutputExecutor::check_err`].
+    pub fn run_shell_command<'a>(
+        &'a self,
+        cmd: &'a str,
+    ) -> ShellOutputExecutor<'a> {
         ShellOutputExecutor {
-            fut,
+            vm: self,
+            cmd,
+            status_check: Some(StatusCheck::Ok),
         }
     }
 

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, ShellOutput},
+    test_vm::{DiskBackend, DiskInterface},
 };
 use phd_testcase::*;
 use std::io::Cursor;
@@ -20,18 +20,23 @@ use efi_utils::{
     EDK2_FIRMWARE_VOL_GUID, EDK2_UI_APP_GUID,
 };
 
+// NOTE: This function differs from `run_shell_command` in that it implicitly
+// ignores the status of executed commands. When
+// https://github.com/oxidecomputer/propolis/issues/773 is fixed and this is
+// deleted, callers of this function may need to be updated to call
+// `.ignore_status` or `.check_err`
 pub(crate) async fn run_long_command(
     vm: &phd_framework::TestVm,
     cmd: &str,
-) -> Result<ShellOutput, Error> {
+) -> Result<String, 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.
     //
     // I haven't gone and debugged that; instead, chunk the input command up
     // into segments short enough to not wrap when input, put them all in a
     // file, then run the file.
-    vm.run_shell_command("rm cmd").await?;
+    vm.run_shell_command("rm cmd").ignore_status().await?;
     let mut offset = 0;
     // Escape any internal `\`. This isn't comprehensive escaping (doesn't
     // handle \n, for example)..
@@ -48,7 +53,9 @@ pub(crate) async fn run_long_command(
 
         vm.run_shell_command(&format!("echo -n \'{}\' >>cmd", chunk)).await?;
     }
-    vm.run_shell_command(". cmd").await
+    // `ignore_status` because it's a bit cumbersome to wrap this whole thing in
+    // a way that checks statuses,
+    vm.run_shell_command(". cmd").ignore_status().await
 }
 
 // This test checks that with a specified boot order, the guest boots whichever
@@ -285,7 +292,7 @@ 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?.ignore_status();
+        vm.run_shell_command("mount | grep efivarfs").ignore_status().await?;
 
     if !mountline.starts_with("efivarfs on ") {
         warn!(
@@ -299,7 +306,6 @@ 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?.expect_ok()?;
+    let hex = run_long_command(vm, &cmd).await?;
 
     Ok(unhex(&hex))
 }
@@ -345,8 +345,7 @@ pub(crate) async fn write_efivar(
         efipath(varname)
     );
 
-    let attr_read_bytes =
-        run_long_command(vm, &attr_cmd).await?.ignore_status();
+    let attr_read_bytes = run_long_command(vm, &attr_cmd).await?;
     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
@@ -391,7 +390,7 @@ pub(crate) async fn write_efivar(
         efipath(varname)
     );
 
-    let res = run_long_command(vm, &cmd).await?.expect_ok()?;
+    let res = run_long_command(vm, &cmd).await?;
     // If something went sideways and the write failed with something like
     // `invalid argument`...
     if !res.is_empty() {

phd-tests/tests/src/cpuid.rs

@@ -169,8 +169,7 @@ 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?.expect_ok()?;
+    let cpuinfo = vm.run_shell_command("cat /proc/cpuinfo").await?;
     info!(cpuinfo, "/proc/cpuinfo output");
     assert!(cpuinfo.contains(
         std::str::from_utf8(BRAND_STRING).unwrap().trim_matches('\0')