From 7c1582ddb926eb16d1e635b74c94059f8cb8e72b Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 27 Nov 2024 19:28:23 +0000 Subject: [PATCH] PHD: use stty to widen the effective terminal for Linux guests All of PHD's supported Linux guests' serial console drivers assume by default that they are writing to an 80-character-wide terminal. This breaks calls to `run_shell_commands` that are longer than 80 characters, because guests will insert extra spaces and carriage returns to deal with terminal wrapping. Suppress this by adding an `stty` command to these guests' login sequences that tells them the terminal is 10,000 characters wide. Add a framework test to validate this. The new test also works (with no additional changes) for Windows Server 2022. It won't currently work with Server 2016 or 2019, which use the 80x24 terminal backend, so skip this test on these OSes (and add an affordance that allows a test to ask the framework for the default guest OS adapter without actually setting up any VM configuration). With this change, the boot order tests' `run_long_command` function is obsolete, so remove it. Finally, tweak `phd_skip!` so that it can take an arbitrary token sequence as an argument instead of only taking a string literal. This allows the use of `format!` in a skip macro. Tested by running this test with all supported guest OS flavors. --- phd-tests/framework/src/guest_os/alpine.rs | 1 + .../src/guest_os/debian11_nocloud.rs | 1 + phd-tests/framework/src/guest_os/linux.rs | 18 +++++++++ phd-tests/framework/src/guest_os/mod.rs | 8 ++++ .../framework/src/guest_os/ubuntu22_04.rs | 1 + phd-tests/framework/src/lib.rs | 11 ++++++ phd-tests/testcase_macro/src/lib.rs | 2 +- phd-tests/tests/src/boot_order.rs | 36 ++--------------- phd-tests/tests/src/boot_order/efi_utils.rs | 8 ++-- phd-tests/tests/src/framework.rs | 39 ++++++++++++++++++- 10 files changed, 84 insertions(+), 41 deletions(-) create mode 100644 phd-tests/framework/src/guest_os/linux.rs diff --git a/phd-tests/framework/src/guest_os/alpine.rs b/phd-tests/framework/src/guest_os/alpine.rs index d8830c1aa..75a0bc038 100644 --- a/phd-tests/framework/src/guest_os/alpine.rs +++ b/phd-tests/framework/src/guest_os/alpine.rs @@ -15,6 +15,7 @@ impl GuestOs for Alpine { CommandSequenceEntry::write_str("root"), CommandSequenceEntry::wait_for(self.get_shell_prompt()), ]) + .extend(super::linux::stty_enable_long_lines(self)) } fn get_shell_prompt(&self) -> &'static str { diff --git a/phd-tests/framework/src/guest_os/debian11_nocloud.rs b/phd-tests/framework/src/guest_os/debian11_nocloud.rs index 6125cebe4..ab993b3d9 100644 --- a/phd-tests/framework/src/guest_os/debian11_nocloud.rs +++ b/phd-tests/framework/src/guest_os/debian11_nocloud.rs @@ -15,6 +15,7 @@ impl GuestOs for Debian11NoCloud { CommandSequenceEntry::write_str("root"), CommandSequenceEntry::wait_for(self.get_shell_prompt()), ]) + .extend(super::linux::stty_enable_long_lines(self)) } fn get_shell_prompt(&self) -> &'static str { diff --git a/phd-tests/framework/src/guest_os/linux.rs b/phd-tests/framework/src/guest_os/linux.rs new file mode 100644 index 000000000..96f58be2b --- /dev/null +++ b/phd-tests/framework/src/guest_os/linux.rs @@ -0,0 +1,18 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Helper functions for building guest OS adaptations for Linux OSes. + +use super::{CommandSequence, CommandSequenceEntry, GuestOs}; + +/// Yields an `stty` command that tells the guest terminal to behave as though +/// it is 9,999 columns wide. +pub(super) fn stty_enable_long_lines<'a>( + guest_os: &impl GuestOs, +) -> CommandSequence<'a> { + CommandSequence(vec![ + CommandSequenceEntry::write_str("stty -F `tty` cols 9999"), + CommandSequenceEntry::wait_for(guest_os.get_shell_prompt()), + ]) +} diff --git a/phd-tests/framework/src/guest_os/mod.rs b/phd-tests/framework/src/guest_os/mod.rs index 246369a0f..18051328c 100644 --- a/phd-tests/framework/src/guest_os/mod.rs +++ b/phd-tests/framework/src/guest_os/mod.rs @@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize}; mod alpine; mod debian11_nocloud; +mod linux; mod shell_commands; mod ubuntu22_04; mod windows; @@ -60,6 +61,13 @@ impl<'a> CommandSequenceEntry<'a> { pub(super) struct CommandSequence<'a>(pub Vec>); +impl<'a> CommandSequence<'a> { + fn extend(mut self, other: CommandSequence<'a>) -> CommandSequence<'a> { + self.0.extend(other.0); + self + } +} + pub(super) trait GuestOs: Send + Sync { /// Retrieves the command sequence used to wait for the OS to boot and log /// into it. diff --git a/phd-tests/framework/src/guest_os/ubuntu22_04.rs b/phd-tests/framework/src/guest_os/ubuntu22_04.rs index d6200e81e..85dcece44 100644 --- a/phd-tests/framework/src/guest_os/ubuntu22_04.rs +++ b/phd-tests/framework/src/guest_os/ubuntu22_04.rs @@ -18,6 +18,7 @@ impl GuestOs for Ubuntu2204 { CommandSequenceEntry::write_str("1!Passw0rd"), CommandSequenceEntry::wait_for(self.get_shell_prompt()), ]) + .extend(super::linux::stty_enable_long_lines(self)) } fn get_shell_prompt(&self) -> &'static str { diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index 1b63dd6d1..a3f2f5e58 100644 --- a/phd-tests/framework/src/lib.rs +++ b/phd-tests/framework/src/lib.rs @@ -35,6 +35,7 @@ use camino::Utf8PathBuf; use disk::DiskFactory; use futures::{stream::FuturesUnordered, StreamExt}; +use guest_os::GuestOsKind; use port_allocator::PortAllocator; use server_log_mode::ServerLogMode; pub use test_vm::TestVm; @@ -304,6 +305,16 @@ impl Framework { &self.default_guest_os_artifact } + /// Yields the guest OS adapter corresponding to the default guest OS + /// artifact. + pub async fn default_guest_os_kind(&self) -> anyhow::Result { + Ok(self + .artifact_store + .get_guest_os_image(&self.default_guest_os_artifact) + .await? + .1) + } + /// Indicates whether the disk factory in this framework supports the /// creation of Crucible disks. This can be used to skip tests that require /// Crucible support. diff --git a/phd-tests/testcase_macro/src/lib.rs b/phd-tests/testcase_macro/src/lib.rs index 85d549438..5a60cae10 100644 --- a/phd-tests/testcase_macro/src/lib.rs +++ b/phd-tests/testcase_macro/src/lib.rs @@ -85,7 +85,7 @@ pub fn phd_skip(args: TokenStream) -> TokenStream { let args = if args.is_empty() { None } else { - let lit = parse_macro_input!(args as syn::Lit); + let lit = parse_macro_input!(args as proc_macro2::TokenStream); Some(lit) }; diff --git a/phd-tests/tests/src/boot_order.rs b/phd-tests/tests/src/boot_order.rs index 0e5af8f2b..feeaafc0e 100644 --- a/phd-tests/tests/src/boot_order.rs +++ b/phd-tests/tests/src/boot_order.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use anyhow::{bail, Error}; +use anyhow::bail; use phd_framework::{ disk::{fat::FatFilesystem, DiskSource}, test_vm::{DiskBackend, DiskInterface}, @@ -20,37 +20,6 @@ use efi_utils::{ EDK2_FIRMWARE_VOL_GUID, EDK2_UI_APP_GUID, }; -pub(crate) async fn run_long_command( - vm: &phd_framework::TestVm, - cmd: &str, -) -> Result { - // 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?; - let mut offset = 0; - // Escape any internal `\`. This isn't comprehensive escaping (doesn't - // handle \n, for example).. - let cmd = cmd.replace("\\", "\\\\"); - while offset < cmd.len() { - let lim = std::cmp::min(cmd.len() - offset, 50); - let chunk = &cmd[offset..][..lim]; - offset += lim; - - // Catch this before it causes weird issues in half-executed commands. - // - // Could escape these here, but right now that's not really necessary. - assert!(!chunk.contains("\n")); - - vm.run_shell_command(&format!("echo -n \'{}\' >>cmd", chunk)).await?; - } - vm.run_shell_command(". cmd").await -} - // This test checks that with a specified boot order, the guest boots whichever // disk we wanted to come first. This is simple enough, until you want to know // "what you booted from".. @@ -305,7 +274,8 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) { // Try adding a few new boot options, then add them to the boot order, // reboot, and make sure they're all as we set them. - if !run_long_command(&vm, &format!("ls {}", efipath(&bootvar(0xffff)))) + if !vm + .run_shell_command(&format!("ls {}", efipath(&bootvar(0xffff)))) .await? .is_empty() { diff --git a/phd-tests/tests/src/boot_order/efi_utils.rs b/phd-tests/tests/src/boot_order/efi_utils.rs index adf1c1f50..20c46b9b4 100644 --- a/phd-tests/tests/src/boot_order/efi_utils.rs +++ b/phd-tests/tests/src/boot_order/efi_utils.rs @@ -20,8 +20,6 @@ use std::fmt::Write; use std::io::{Cursor, Read}; use tracing::{info, trace, warn}; -use super::run_long_command; - // First, some GUIDs. These GUIDs come from EDK2, and OVMF reuses them. Notably // these are the raw bytes of the GUID: textual values will have slightly // different ordering of bytes. @@ -326,7 +324,7 @@ pub(crate) async fn read_efivar( efipath(varname) ); - let hex = run_long_command(vm, &cmd).await?; + let hex = vm.run_shell_command(&cmd).await?; Ok(unhex(&hex)) } @@ -345,7 +343,7 @@ pub(crate) async fn write_efivar( efipath(varname) ); - let attr_read_bytes = run_long_command(vm, &attr_cmd).await?; + let attr_read_bytes = vm.run_shell_command(&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 @@ -390,7 +388,7 @@ pub(crate) async fn write_efivar( efipath(varname) ); - let res = run_long_command(vm, &cmd).await?; + let res = vm.run_shell_command(&cmd).await?; // If something went sideways and the write failed with something like // `invalid argument`... if !res.is_empty() { diff --git a/phd-tests/tests/src/framework.rs b/phd-tests/tests/src/framework.rs index 3f1cb8cab..4fc88f249 100644 --- a/phd-tests/tests/src/framework.rs +++ b/phd-tests/tests/src/framework.rs @@ -2,9 +2,9 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! This module contains tests whose primary goal is to verify the correctness -//! of the PHD framework itself. +//! Tests that primarily exercise the PHD framework itself. +use phd_framework::guest_os::GuestOsKind; use phd_testcase::*; #[phd_testcase] @@ -16,3 +16,38 @@ async fn multiline_serial_test(ctx: &Framework) { let out = vm.run_shell_command("echo \\\nhello \\\nworld").await?; assert_eq!(out, "hello world"); } + +#[phd_testcase] +async fn long_line_serial_test(ctx: &Framework) { + let os = ctx.default_guest_os_kind().await?; + if matches!( + os, + GuestOsKind::WindowsServer2016 | GuestOsKind::WindowsServer2019 + ) { + phd_skip!(format!( + "long serial lines not supported for guest OS {os:?}" + )); + } + + let mut vm = ctx.spawn_default_vm("long_line_serial_test").await?; + vm.launch().await?; + vm.wait_to_boot().await?; + + let long_str = "In my younger and more vulnerable years my father gave \ + me some advice that I've been turning over in my mind ever since. \ + \"Whenever you feel like sending a long serial console line,\" he told me, \ + \"just remember that all the guest OSes in this world haven't had the tty \ + settings you've had.\""; + + let out = vm + .run_shell_command(&format!( + "echo '{}'", + // Fitzgerald didn't have to deal with nested Bash quotes, but this + // test does. Replace apostrophes in the input string with a + // string-terminating `'`, followed by an escaped single quote that + // serves as the apostrophe, followed by a string-opening `'`. + long_str.replace("'", "'\\''") + )) + .await?; + assert_eq!(out, long_str); +}