From 56028fb214e23f21083edd02cf0d53b097e20b07 Mon Sep 17 00:00:00 2001 From: William Douglas Date: Thu, 4 Mar 2021 23:34:45 +0000 Subject: [PATCH] Try to restore pty configuration on reboot When a vm is created with a pty device, on reboot the pty fd (sub only) will only be associated with the vmm through the epoll event loop. The fd being polled will have been closed due to the vm itself dropping the pty files (and potentially reopening the fd index to a different item making things quite confusing) and new pty fds will be opened but not polled on for input. This change creates a structure to encapsulate the information about the pty fd (main File, sub File and the path to the sub File). On reboot, a copy of the console and serial pty structs is then passed down to the new Vm instance which will be used instead of creating a new pty device. This resolves the underlying issue from #2316. Signed-off-by: William Douglas --- vmm/src/device_manager.rs | 96 +++++++++++++++++++++++++++------------ vmm/src/lib.rs | 14 ++++-- vmm/src/vm.rs | 17 ++++--- 3 files changed, 88 insertions(+), 39 deletions(-) diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index af666d182..841349663 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -797,6 +797,23 @@ impl DeviceInfoForFDT for MMIODeviceInfo { } } +#[derive(Debug)] +pub struct PtyPair { + pub main: File, + pub sub: File, + pub path: PathBuf, +} + +impl PtyPair { + fn clone(&self) -> Self { + PtyPair { + main: self.main.try_clone().unwrap(), + sub: self.sub.try_clone().unwrap(), + path: self.path.clone(), + } + } +} + pub struct DeviceManager { // Manage address space related to devices address_manager: Arc, @@ -805,10 +822,10 @@ pub struct DeviceManager { console: Arc, // console PTY - console_pty: Option>>, + console_pty: Option>>, // serial PTY - serial_pty: Option>>, + serial_pty: Option>>, // Interrupt controller #[cfg(target_arch = "x86_64")] @@ -1010,19 +1027,23 @@ impl DeviceManager { Ok(device_manager) } - pub fn serial_pty(&self) -> Option { + pub fn serial_pty(&self) -> Option { self.serial_pty .as_ref() - .map(|pty| pty.lock().unwrap().0.try_clone().unwrap()) + .map(|pty| pty.lock().unwrap().clone()) } - pub fn console_pty(&self) -> Option { + pub fn console_pty(&self) -> Option { self.console_pty .as_ref() - .map(|pty| pty.lock().unwrap().0.try_clone().unwrap()) + .map(|pty| pty.lock().unwrap().clone()) } - pub fn create_devices(&mut self) -> DeviceManagerResult<()> { + pub fn create_devices( + &mut self, + serial_pty: Option, + console_pty: Option, + ) -> DeviceManagerResult<()> { let mut virtio_devices: Vec<(VirtioDeviceArc, bool, String)> = Vec::new(); let interrupt_controller = self.add_interrupt_controller()?; @@ -1071,7 +1092,12 @@ impl DeviceManager { )?; } - self.console = self.add_console_device(&legacy_interrupt_manager, &mut virtio_devices)?; + self.console = self.add_console_device( + &legacy_interrupt_manager, + &mut virtio_devices, + serial_pty, + console_pty, + )?; // Reserve some IRQs for PCI devices in case they need to support INTx. self.reserve_legacy_interrupts_for_pci_devices()?; @@ -1664,6 +1690,8 @@ impl DeviceManager { &mut self, interrupt_manager: &Arc>, virtio_devices: &mut Vec<(VirtioDeviceArc, bool, String)>, + serial_pty: Option, + console_pty: Option, ) -> DeviceManagerResult> { let serial_config = self.config.lock().unwrap().serial.clone(); let serial_writer: Option> = match serial_config.mode { @@ -1672,16 +1700,21 @@ impl DeviceManager { .map_err(DeviceManagerError::SerialOutputFileOpen)?, )), ConsoleOutputMode::Pty => { - let (main, mut sub, path) = - create_pty().map_err(DeviceManagerError::SerialPtyOpen)?; - self.set_raw_mode(&mut sub) - .map_err(DeviceManagerError::SetPtyRaw)?; - self.serial_pty = Some(Arc::new(Mutex::new(( - main.try_clone().unwrap(), - sub.try_clone().unwrap(), - )))); - self.config.lock().unwrap().serial.file = Some(path); - Some(Box::new(main.try_clone().unwrap())) + if let Some(pty) = serial_pty { + self.config.lock().unwrap().serial.file = Some(pty.path.clone()); + let writer = pty.main.try_clone().unwrap(); + self.serial_pty = Some(Arc::new(Mutex::new(pty))); + Some(Box::new(writer)) + } else { + let (main, mut sub, path) = + create_pty().map_err(DeviceManagerError::SerialPtyOpen)?; + self.set_raw_mode(&mut sub) + .map_err(DeviceManagerError::SetPtyRaw)?; + self.config.lock().unwrap().serial.file = Some(path.clone()); + let writer = main.try_clone().unwrap(); + self.serial_pty = Some(Arc::new(Mutex::new(PtyPair { main, sub, path }))); + Some(Box::new(writer)) + } } ConsoleOutputMode::Tty => Some(Box::new(stdout())), ConsoleOutputMode::Off | ConsoleOutputMode::Null => None, @@ -1700,16 +1733,21 @@ impl DeviceManager { .map_err(DeviceManagerError::ConsoleOutputFileOpen)?, )), ConsoleOutputMode::Pty => { - let (main, mut sub, path) = - create_pty().map_err(DeviceManagerError::SerialPtyOpen)?; - self.set_raw_mode(&mut sub) - .map_err(DeviceManagerError::SetPtyRaw)?; - self.console_pty = Some(Arc::new(Mutex::new(( - main.try_clone().unwrap(), - sub.try_clone().unwrap(), - )))); - self.config.lock().unwrap().console.file = Some(path); - Some(Box::new(main.try_clone().unwrap())) + if let Some(pty) = console_pty { + self.config.lock().unwrap().console.file = Some(pty.path.clone()); + let writer = pty.main.try_clone().unwrap(); + self.console_pty = Some(Arc::new(Mutex::new(pty))); + Some(Box::new(writer)) + } else { + let (main, mut sub, path) = + create_pty().map_err(DeviceManagerError::ConsolePtyOpen)?; + self.set_raw_mode(&mut sub) + .map_err(DeviceManagerError::SetPtyRaw)?; + self.config.lock().unwrap().console.file = Some(path.clone()); + let writer = main.try_clone().unwrap(); + self.console_pty = Some(Arc::new(Mutex::new(PtyPair { main, sub, path }))); + Some(Box::new(writer)) + } } ConsoleOutputMode::Tty => Some(Box::new(stdout())), ConsoleOutputMode::Null => Some(Box::new(sink())), @@ -3802,7 +3840,7 @@ impl Snapshottable for DeviceManager { // Now that DeviceManager is updated with the right states, it's time // to create the devices based on the configuration. - self.create_devices() + self.create_devices(None, None) .map_err(|e| MigratableError::Restore(anyhow!("Could not create devices {:?}", e)))?; // Finally, restore all devices associated with the DeviceManager. diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 42fea0f6f..f76e58b9f 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -359,15 +359,17 @@ impl Vmm { &self.seccomp_action, self.hypervisor.clone(), activate_evt, + None, + None, )?; - if let Some(ref serial_pty) = vm.serial_pty() { + if let Some(serial_pty) = vm.serial_pty() { self.epoll - .add_event(serial_pty, EpollDispatch::Pty) + .add_event(&serial_pty.main, EpollDispatch::Pty) .map_err(VmError::EventfdError)?; }; - if let Some(ref console_pty) = vm.console_pty() { + if let Some(console_pty) = vm.console_pty() { self.epoll - .add_event(console_pty, EpollDispatch::Pty) + .add_event(&console_pty.main, EpollDispatch::Pty) .map_err(VmError::EventfdError)?; }; self.vm = Some(vm); @@ -477,6 +479,8 @@ impl Vmm { // First we stop the current VM and create a new one. if let Some(ref mut vm) = self.vm { let config = vm.get_config(); + let serial_pty = vm.serial_pty(); + let console_pty = vm.console_pty(); self.vm_shutdown()?; let exit_evt = self.exit_evt.try_clone().map_err(VmError::EventFdClone)?; @@ -499,6 +503,8 @@ impl Vmm { &self.seccomp_action, self.hypervisor.clone(), activate_evt, + serial_pty, + console_pty, )?); } diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index c02b5bcdc..ce30901f1 100644 --- a/vmm/src/vm.rs +++ b/vmm/src/vm.rs @@ -29,7 +29,9 @@ use crate::config::{ VmConfig, VsockConfig, }; use crate::cpu; -use crate::device_manager::{self, get_win_size, Console, DeviceManager, DeviceManagerError}; +use crate::device_manager::{ + self, get_win_size, Console, DeviceManager, DeviceManagerError, PtyPair, +}; use crate::device_tree::DeviceTree; use crate::memory_manager::{Error as MemoryManagerError, MemoryManager}; use crate::migration::{get_vm_snapshot, url_to_path, VM_SNAPSHOT_FILE}; @@ -650,6 +652,7 @@ impl Vm { Ok(numa_nodes) } + #[allow(clippy::too_many_arguments)] pub fn new( config: Arc>, exit_evt: EventFd, @@ -657,6 +660,8 @@ impl Vm { seccomp_action: &SeccompAction, hypervisor: Arc, activate_evt: EventFd, + serial_pty: Option, + console_pty: Option, ) -> Result { #[cfg(all(feature = "kvm", target_arch = "x86_64"))] hypervisor.check_required_extensions().unwrap(); @@ -702,7 +707,7 @@ impl Vm { .device_manager .lock() .unwrap() - .create_devices() + .create_devices(serial_pty, console_pty) .map_err(Error::DeviceManager)?; Ok(new_vm) } @@ -1068,11 +1073,11 @@ impl Vm { Ok(()) } - pub fn serial_pty(&self) -> Option { + pub fn serial_pty(&self) -> Option { self.device_manager.lock().unwrap().serial_pty() } - pub fn console_pty(&self) -> Option { + pub fn console_pty(&self) -> Option { self.device_manager.lock().unwrap().console_pty() } @@ -1574,7 +1579,7 @@ impl Vm { let dm = self.device_manager.lock().unwrap(); let mut out = [0u8; 64]; if let Some(mut pty) = dm.serial_pty() { - let count = pty.read(&mut out).map_err(Error::PtyConsole)?; + let count = pty.main.read(&mut out).map_err(Error::PtyConsole)?; let console = dm.console(); if console.input_enabled() { console @@ -1583,7 +1588,7 @@ impl Vm { } }; let count = match dm.console_pty() { - Some(mut pty) => pty.read(&mut out).map_err(Error::PtyConsole)?, + Some(mut pty) => pty.main.read(&mut out).map_err(Error::PtyConsole)?, None => return Ok(()), }; let console = dm.console();