Skip to content

Commit

Permalink
leave the typical/implicit "command exits 0" behavior intact
Browse files Browse the repository at this point in the history 
this makes `run_shell_command` a bit more magic but keeps tests less
boilerplate-y. tyvm Eliza for helping me figure out how to put the
future together...
  • Loading branch information
@iximeow
iximeow committed Oct 18, 2024
1 parent 35b1ac9 commit 3c5f16c
Show file tree
Hide file tree
Showing 11 changed files with 162 additions and 201 deletions.
226 changes: 102 additions & 124 deletions phd-tests/framework/src/test_vm/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -148,134 +148,113 @@ pub struct ShellOutput {
output: String,
}

/// 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 ShellOutputExecutor<'ctx> {
vm: &'ctx TestVm,
cmd: &'ctx str,
status_check: Option<StatusCheck>,
}

/*
impl ShellOutputExecutor {
fn require_ok(self) -> ShellOutputOkExecutor {
ShellOutputOkExecutor {
fut: self.fut
}
impl<'a> ShellOutputExecutor<'a> {
pub fn ignore_status(mut self) -> ShellOutputExecutor<'a> {
self.status_check = None;
self
}
}
*/

impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> {
type Output = Result<ShellOutput>;
type IntoFuture = Pin<Box<dyn std::future::Future<Output = Self::Output>>>;

fn into_future(self) -> Self::IntoFuture {
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?;

let status_command_sequence =
self.vm.guest_os.shell_command_sequence("echo $?");
self.vm.run_command_sequence(status_command_sequence).await?;

let status = self
.vm
.wait_for_serial_output(
self.vm.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 })
})
pub fn check_ok(mut self) -> ShellOutputExecutor<'a> {
self.status_check = Some(StatusCheck::Ok);
self
}
}

pub struct ShellOutputOkExecutor {
fut: Pin<Box<dyn std::future::Future<Output=Result<ShellOutput>>>>
}

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)
pub fn check_err(mut self) -> ShellOutputExecutor<'a> {
self.status_check = Some(StatusCheck::NotOk);
self
}
}
*/
use futures::FutureExt;

impl std::future::Future for ShellOutputOkExecutor {
impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> {
type Output = Result<String>;
type IntoFuture = futures::future::BoxFuture<'a, Result<String>>;

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,
}
}
}

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
}
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?;

pub fn status(&self) -> u16 {
self.status
}
let status_command_sequence =
self.vm.guest_os.shell_command_sequence("echo $?");
self.vm.run_command_sequence(status_command_sequence).await?;

/// 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()
let status = self
.vm
.wait_for_serial_output(
self.vm.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 })
});
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()
}
}

Expand Down Expand Up @@ -1008,20 +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) -> ShellOutputExecutor<'a> {
///
/// 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 {
vm: self,
cmd,
status_check: Some(StatusCheck::Ok),
}
}

Expand Down
18 changes: 12 additions & 6 deletions phd-tests/tests/src/boot_order.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)..
Expand All @@ -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
Expand Down Expand Up @@ -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!(
Expand All @@ -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!(
Expand Down
7 changes: 3 additions & 4 deletions phd-tests/tests/src/boot_order/efi_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand All @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
3 changes: 1 addition & 2 deletions phd-tests/tests/src/cpuid.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
Loading

0 comments on commit 3c5f16c

Please sign in to comment.