PHD: make callers consume serial buffer by hand

Change the semantics of waiting for characters in the serial buffer so that
waits return the preceding buffer contents but don't actually erase anything
from the buffer. Instead, make callers manually issue a "clear" command to
discard from the buffer anything they know they're not going to want, and update
the guest shell command sequences to clear the buffer between echoing the entire
command and pressing Enter to execute it.

This fixes issues with command echoing for Windows guests that don't redraw
characters that the guest believes are already on the screen--now those
characters don't get consumed from the buffer, so it's OK that the guest doesn't
explicitly reprint them.

Author: gjcolombo

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

@@ -27,6 +27,9 @@ pub(super) enum CommandSequenceEntry<'a> {
     /// Write the specified string as a command to the guest serial console.
     WriteStr(Cow<'a, str>),
 
+    /// Tell the serial console task to clear its buffer.
+    ClearBuffer,
+
     /// Change the serial console buffering discipline to the supplied
     /// discipline.
     ChangeSerialConsoleBuffer(crate::serial::BufferKind),

phd-tests/framework/src/guest_os/shell_commands.rs

@@ -25,6 +25,7 @@ pub(super) fn shell_command_sequence(
         crate::serial::BufferKind::Raw => CommandSequence(vec![
             CommandSequenceEntry::WriteStr(cmd),
             CommandSequenceEntry::WaitFor(echo.into()),
+            CommandSequenceEntry::ClearBuffer,
             CommandSequenceEntry::WriteStr("\n".into()),
         ]),
 
@@ -37,12 +38,22 @@ pub(super) fn shell_command_sequence(
             let cmd_lines = cmd.trim_end().lines();
             let echo_lines = echo.lines();
             let mut seq = vec![];
-            for (cmd, echo) in cmd_lines.zip(echo_lines) {
+
+            let mut iter = cmd_lines.zip(echo_lines).peekable();
+            while let Some((cmd, echo)) = iter.next() {
                 seq.push(CommandSequenceEntry::WriteStr(cmd.to_owned().into()));
                 seq.push(CommandSequenceEntry::WaitFor(echo.to_owned().into()));
-                seq.push(CommandSequenceEntry::WriteStr("\n".into()));
+
+                if iter.peek().is_some() {
+                    seq.push(CommandSequenceEntry::WriteStr("\n".into()));
+                }
             }
 
+            // Before issuing the command, clear any stale echoed characters
+            // from the serial console buffer. This ensures that the
+            seq.push(CommandSequenceEntry::ClearBuffer);
+            seq.push(CommandSequenceEntry::WriteStr("\n".into()));
+
             CommandSequence(seq)
         }
     }

phd-tests/framework/src/guest_os/windows_server_2016.rs

@@ -32,6 +32,10 @@ impl GuestOs for WindowsServer2016 {
     }
 
     fn shell_command_sequence<'a>(&self, cmd: &'a str) -> CommandSequence<'a> {
+        // `reset` the command prompt before issuing the command to try to force
+        // Windows to redraw the subsequent command prompt. Without this,
+        // Windows may not draw the prompt if the post-command state happens to
+        // place a prompt at a location that already had one pre-command.
         let cmd = format!("reset && {cmd}");
         super::shell_commands::shell_command_sequence(
             Cow::Owned(cmd),

phd-tests/framework/src/guest_os/windows_server_2019.rs

@@ -28,6 +28,10 @@ impl GuestOs for WindowsServer2019 {
     }
 
     fn shell_command_sequence<'a>(&self, cmd: &'a str) -> CommandSequence<'a> {
+        // `reset` the command prompt before issuing the command to try to force
+        // Windows to redraw the subsequent command prompt. Without this,
+        // Windows may not draw the prompt if the post-command state happens to
+        // place a prompt at a location that already had one pre-command.
         let cmd = format!("reset && {cmd}");
         super::shell_commands::shell_command_sequence(
             Cow::Owned(cmd),

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

@@ -35,6 +35,9 @@ trait Buffer: Send {
     /// Processes the supplied `bytes` as input to the buffer.
     fn process_bytes(&mut self, bytes: &[u8]);
 
+    /// Clears the unprocessed contents of the buffer.
+    fn clear(&mut self);
+
     /// Registers a new request to wait for a string to appear in the buffer.
     fn register_wait_for_output(&mut self, waiter: OutputWaiter);
 
@@ -61,6 +64,10 @@ enum TaskCommand {
     /// Send the supplied bytes to the VM.
     SendBytes { bytes: Vec<u8>, done: oneshot::Sender<()> },
 
+    /// Clears the contents of the task's console buffer. This does not cancel
+    /// the active wait, if there is one.
+    Clear,
+
     /// Register to be notified if and when a supplied string appears in the
     /// serial console's buffer.
     RegisterWait(OutputWaiter),
@@ -117,12 +124,21 @@ impl SerialConsole {
         Ok(())
     }
 
+    /// Directs the console worker thread to clear the serial console buffer.
+    pub fn clear(&self) -> anyhow::Result<()> {
+        self.cmd_tx.send(TaskCommand::Clear)?;
+        Ok(())
+    }
+
     /// Registers with the current buffer a request to wait for `wanted` to
     /// appear in the console buffer. When a match is found, the buffer sends
-    /// all buffered characters preceding the match to `preceding_tx`, consuming
-    /// those characters and the matched string. If the buffer already contains
-    /// one or more matches at the time the waiter is registered, the last match
-    /// is used to satisfy the wait immediately.
+    /// all buffered characters preceding the match to `preceding_tx`. If the
+    /// buffer already contains one or more matches at the time the waiter is
+    /// registered, the last match is used to satisfy the wait immediately.
+    ///
+    /// Note that this function *does not* clear any characters from the buffer.
+    /// Callers who want to retire previously-echoed characters in the buffer
+    /// must explicitly call `clear`.
     pub fn register_wait_for_string(
         &self,
         wanted: String,
@@ -225,6 +241,7 @@ async fn serial_task(
 
                         let _ = done.send(());
                     }
+                    TaskCommand::Clear => buffer.clear(),
                     TaskCommand::RegisterWait(waiter) => {
                         buffer.register_wait_for_output(waiter);
                     }

phd-tests/framework/src/serial/raw_buffer.rs

@@ -87,19 +87,14 @@ impl RawBuffer {
     fn satisfy_or_set_wait(&mut self, waiter: OutputWaiter) {
         assert!(self.waiter.is_none());
         if let Some(idx) = self.wait_buffer.rfind(&waiter.wanted) {
-            // Send all of the data in the buffer prior to the target string
-            // out the waiter's channel.
-            //
+            let out = self.wait_buffer[..idx].to_owned();
+
             // Because incoming bytes from Propolis may be processed on a
             // separate task than the task that registered the wait, this
             // can race such that the wait is satisfied just as the waiter
             // times out and closes its half of the channel. There's nothing
             // to be done about this, so just ignore any errors here.
-            let out = self.wait_buffer.drain(..idx).collect();
             let _ = waiter.preceding_tx.send(out);
-
-            // Clear the matched string out of the wait buffer.
-            self.wait_buffer = self.wait_buffer.split_off(waiter.wanted.len());
         } else {
             self.waiter = Some(waiter);
         }
@@ -126,6 +121,10 @@ impl Buffer for RawBuffer {
         }
     }
 
+    fn clear(&mut self) {
+        self.wait_buffer.clear();
+    }
+
     fn register_wait_for_output(&mut self, waiter: OutputWaiter) {
         self.satisfy_or_set_wait(waiter);
     }

phd-tests/framework/src/serial/vt80x24.rs

@@ -110,47 +110,6 @@ impl Vt80x24 {
         let mut contents = self.surface.screen_chars_to_string();
         trace!(?contents, "termwiz contents");
         if let Some(idx) = contents.rfind(&waiter.wanted) {
-            // Callers who set waits assume that matched strings are consumed
-            // from the buffer and won't match again unless the string is
-            // rendered to the buffer again. In the raw buffering case, this is
-            // straightforward: the buffer is already a String, so it suffices
-            // just to split the string just after the match. In this case,
-            // however, life is harder, because the buffer is not a string but a
-            // collection of cells containing `char`s.
-            //
-            // To "consume" the buffer, overwrite everything up through the
-            // match string with spaces. Start by truncating the current buffer
-            // contents down to the substring that ends in the match.
-            let last_byte = idx + waiter.wanted.len();
-            contents.truncate(last_byte);
-
-            // Then move the cursor to the top left and "type" as many blank
-            // spaces as there were characters in the buffer. Note that termwiz
-            // inserts extra '\n' characters at the end of every line that need
-            // to be ignored here. (It's assumed that none of the actual
-            // terminal cells contain a '\n' control character--if one of those
-            // is printed the cursor moves instead.)
-            let char_count = contents.chars().filter(|c| *c != '\n').count();
-
-            // Before typing anything, remember the old cursor position so that
-            // it can be restored after typing the spaces.
-            //
-            // It's insufficient to assume that the last character of the match
-            // was actually "typed" such that the cursor advanced past it. For
-            // example, if a match string ends with "$ ", and the guest moves to
-            // the start of an empty line and types a single "$", the cursor is
-            // in column 1 (just past the "$"), but the match is satisfied by
-            // the "pre-existing" space in that column.
-            let (old_col, old_row) = self.surface.cursor_position();
-            self.surface.add_change(make_absolute_cursor_position(0, 0));
-            self.surface.add_change(Change::Text(" ".repeat(char_count)));
-            let seq = self
-                .surface
-                .add_change(make_absolute_cursor_position(old_col, old_row));
-            self.surface.flush_changes_older_than(seq);
-
-            // Remove the match string from the match contents and push
-            // everything else back to the listener.
             contents.truncate(idx);
             let _ = waiter.preceding_tx.send(contents);
         } else {
@@ -165,6 +124,13 @@ impl Buffer for Vt80x24 {
         self.apply_actions(&actions);
     }
 
+    fn clear(&mut self) {
+        let seq = self
+            .surface
+            .add_change(Change::ClearScreen(ColorAttribute::Default));
+        self.surface.flush_changes_older_than(seq);
+    }
+
     fn register_wait_for_output(&mut self, waiter: OutputWaiter) {
         self.satisfy_or_set_wait(waiter);
     }

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

@@ -659,6 +659,9 @@ impl TestVm {
                         self.send_serial_str(s.as_ref()).await?;
                         self.send_serial_str("\n").await?;
                     }
+                    CommandSequenceEntry::ClearBuffer => {
+                        self.clear_serial_buffer()?
+                    }
                     CommandSequenceEntry::ChangeSerialConsoleBuffer(kind) => {
                         self.change_serial_buffer_kind(kind)?;
                     }
@@ -686,8 +689,8 @@ impl TestVm {
     }
 
     /// Waits for up to `timeout_duration` for `line` to appear on the guest
-    /// serial console, then returns the unconsumed portion of the serial
-    /// console buffer that preceded the requested string.
+    /// serial console, then returns the contents of the console buffer that
+    /// preceded the requested string.
     #[instrument(skip_all, fields(vm = self.spec.vm_name, vm_id = %self.id))]
     pub async fn wait_for_serial_output(
         &self,
@@ -746,6 +749,9 @@ impl TestVm {
                 CommandSequenceEntry::WriteStr(s) => {
                     self.send_serial_str(s.as_ref()).await?;
                 }
+                CommandSequenceEntry::ClearBuffer => {
+                    self.clear_serial_buffer()?
+                }
                 _ => {
                     anyhow::bail!(
                         "Unexpected command sequence entry {step:?} while \
@@ -792,6 +798,13 @@ impl TestVm {
         }
     }
 
+    fn clear_serial_buffer(&self) -> Result<()> {
+        match &self.state {
+            VmState::Ensured { serial } => serial.clear(),
+            VmState::New => Err(VmStateError::InstanceNotEnsured.into()),
+        }
+    }
+
     fn change_serial_buffer_kind(&self, kind: BufferKind) -> Result<()> {
         match &self.state {
             VmState::Ensured { serial } => serial.change_buffer_kind(kind),