From 9d5e8f34ade78222a8a4ab0a6fb259e2c2d1faa3 Mon Sep 17 00:00:00 2001 From: Jingkui Wang Date: Thu, 1 Nov 2018 16:53:47 +0000 Subject: [PATCH] Revert "devices: refactor proxy to use msg_socket" This reverts commit 142ce3efd9c6a20efb4685fa632ad522509441f2. Reason for revert: Original change's description: > devices: refactor proxy to use msg_socket > > Use msg socket in proxy. > > BUG=None > TEST=None > > Change-Id: Ia5ebc4410918a261fe525abc1051ebbbdc66a876 > Reviewed-on: https://chromium-review.googlesource.com/1260259 > Commit-Ready: Jingkui Wang > Tested-by: Jingkui Wang > Reviewed-by: Zach Reizner Bug: None Change-Id: Ic7827969e9ad508cd1b65cb7b8747e81e0cd02d0 Reviewed-on: https://chromium-review.googlesource.com/c/1313014 Reviewed-by: Daniel Verkamp Commit-Queue: ChromeOS CL Exonerator Bot Tested-by: Jingkui Wang --- Cargo.lock | 1 - devices/Cargo.toml | 1 - devices/src/lib.rs | 2 - devices/src/proxy.rs | 235 +++++++++++++++++++++++-------------------- 4 files changed, 125 insertions(+), 114 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b9705f9879..15720da787 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -135,7 +135,6 @@ dependencies = [ "io_jail 0.1.0", "kvm 0.1.0", "libc 0.2.40 (registry+https://github.com/rust-lang/crates.io-index)", - "msg_socket 0.1.0", "net_sys 0.1.0", "net_util 0.1.0", "p9 0.1.0", diff --git a/devices/Cargo.toml b/devices/Cargo.toml index b6c94a2e8c..ce0c0e59b4 100644 --- a/devices/Cargo.toml +++ b/devices/Cargo.toml @@ -15,7 +15,6 @@ gpu_display = { path = "../gpu_display", optional = true } gpu_renderer = { path = "../gpu_renderer", optional = true } kvm = { path = "../kvm" } libc = "*" -msg_socket = { path = "../msg_socket" } io_jail = { path = "../io_jail" } net_sys = { path = "../net_sys" } net_util = { path = "../net_util" } diff --git a/devices/src/lib.rs b/devices/src/lib.rs index a8e89a45e8..d76e7439dd 100644 --- a/devices/src/lib.rs +++ b/devices/src/lib.rs @@ -18,8 +18,6 @@ extern crate sys_util; extern crate vhost; extern crate virtio_sys; extern crate vm_control; -#[macro_use] -extern crate msg_socket; mod bus; mod cmos; diff --git a/devices/src/proxy.rs b/devices/src/proxy.rs index 86fcd3343e..5b79d31c89 100644 --- a/devices/src/proxy.rs +++ b/devices/src/proxy.rs @@ -12,7 +12,7 @@ use std::process; use std::time::Duration; use std::{self, fmt, io}; -use msg_socket::{MsgError, MsgOnSocket, MsgReceiver, MsgResult, MsgSender, MsgSocket}; +use byteorder::{ByteOrder, LittleEndian, NativeEndian}; use io_jail::{self, Minijail}; use BusDevice; @@ -35,80 +35,74 @@ impl fmt::Display for Error { } const SOCKET_TIMEOUT_MS: u64 = 2000; +const MSG_SIZE: usize = 24; -#[derive(MsgOnSocket)] enum Command { - Read { - len: u32, - offset: u64, - }, - Write { - len: u32, - offset: u64, - data: [u8; 8], - }, - ReadConfig(u32), - WriteConfig { - reg_idx: u32, - offset: u32, - len: u32, - data: [u8; 4], - }, - Shutdown, -} - -#[derive(MsgOnSocket)] -enum CommandResult { - Ok, - ReadResult([u8; 8]), - ReadConfigResult(u32), + Read = 0, + Write = 1, + ReadConfig = 2, + WriteConfig = 3, + Shutdown = 4, } fn child_proc(sock: UnixDatagram, device: &mut BusDevice) { let mut running = true; - let sock = MsgSocket::::new(sock); while running { - let cmd = match sock.recv() { - Ok(cmd) => cmd, - Err(err) => { - error!("child device process failed recv: {:?}", err); + let mut buf = [0; MSG_SIZE]; + match handle_eintr!(sock.recv(&mut buf)) { + Ok(c) if c != buf.len() => { + error!( + "child device process incorrect recv size: got {}, expected {}", + c, + buf.len() + ); break; } + Err(e) => { + error!("child device process failed recv: {}", e); + break; + } + _ => {} + } + + let cmd = NativeEndian::read_u32(&buf[0..]); + + let res = if cmd == Command::Read as u32 { + let len = NativeEndian::read_u32(&buf[4..]) as usize; + let offset = NativeEndian::read_u64(&buf[8..]); + device.read(offset, &mut buf[16..16 + len]); + handle_eintr!(sock.send(&buf)) + } else if cmd == Command::Write as u32 { + let len = NativeEndian::read_u32(&buf[4..]) as usize; + let offset = NativeEndian::read_u64(&buf[8..]); + device.write(offset, &buf[16..16 + len]); + handle_eintr!(sock.send(&buf)) + } else if cmd == Command::ReadConfig as u32 { + let reg_idx = NativeEndian::read_u32(&buf[4..]) as usize; + let val = device.config_register_read(reg_idx); + buf[16] = val as u8; + buf[17] = (val >> 8) as u8; + buf[18] = (val >> 16) as u8; + buf[19] = (val >> 24) as u8; + handle_eintr!(sock.send(&buf)) + } else if cmd == Command::WriteConfig as u32 { + let reg_idx = NativeEndian::read_u32(&buf[4..]) as usize; + let offset = u64::from(NativeEndian::read_u32(&buf[8..])); + let len = u64::from(NativeEndian::read_u32(&buf[16..])); + device.config_register_write(reg_idx, offset, &buf[20..(20 + len as usize)]); + handle_eintr!(sock.send(&buf)) + } else if cmd == Command::Shutdown as u32 { + running = false; + handle_eintr!(sock.send(&buf)) + } else { + error!("child device process unknown command: {}", cmd); + break; }; - let res = match cmd { - Command::Read { len, offset } => { - let mut buffer = [0u8; 8]; - device.read(offset, &mut buffer[0..len as usize]); - sock.send(&CommandResult::ReadResult(buffer)) - } - Command::Write { len, offset, data } => { - let len = len as usize; - device.write(offset, &data[0..len]); - sock.send(&CommandResult::Ok) - } - Command::ReadConfig(idx) => { - let val = device.config_register_read(idx as usize); - sock.send(&CommandResult::ReadConfigResult(val)) - } - Command::WriteConfig { - reg_idx, - offset, - len, - data, - } => { - let len = len as usize; - device.config_register_write(reg_idx as usize, offset as u64, &data[0..len]); - sock.send(&CommandResult::Ok) - } - Command::Shutdown => { - running = false; - sock.send(&CommandResult::Ok) - } - }; if let Err(e) = res { - error!("child device process failed send: {:?}", e); + error!("error: child device process failed send: {}", e); + break; } } } @@ -118,7 +112,7 @@ fn child_proc(sock: UnixDatagram, device: &mut BusDevice) { /// Because forks are very unfriendly to destructors and all memory mappings and file descriptors /// are inherited, this should be used as early as possible in the main process. pub struct ProxyDevice { - sock: MsgSocket, + sock: UnixDatagram, pid: pid_t, } @@ -158,7 +152,7 @@ impl ProxyDevice { .set_read_timeout(Some(Duration::from_millis(SOCKET_TIMEOUT_MS))) .map_err(Error::Io)?; Ok(ProxyDevice { - sock: MsgSocket::::new(parent_sock), + sock: parent_sock, pid, }) } @@ -167,69 +161,90 @@ impl ProxyDevice { self.pid } - fn sync_send(&self, cmd: Command) -> Option { - let res = self.sock.send(&cmd); - if let Err(e) = res { - error!("failed write to child device process: {:?}", e); - }; - match self.sock.recv() { - Err(e) => { - error!("failed read from child device process: {:?}", e); - None - } - Ok(r) => Some(r), - } + fn send_cmd(&self, cmd: Command, offset: u64, len: u32, data: &[u8]) -> Result<()> { + let mut buf = [0; MSG_SIZE]; + NativeEndian::write_u32(&mut buf[0..], cmd as u32); + NativeEndian::write_u32(&mut buf[4..], len); + NativeEndian::write_u64(&mut buf[8..], offset); + buf[16..16 + data.len()].clone_from_slice(data); + handle_eintr!(self.sock.send(&buf)) + .map(|_| ()) + .map_err(Error::Io) + } + + fn send_config_cmd(&self, cmd: Command, reg_idx: u32, offset: u64, data: &[u8]) -> Result<()> { + let mut buf = [0; MSG_SIZE]; + NativeEndian::write_u32(&mut buf[0..], cmd as u32); + NativeEndian::write_u32(&mut buf[4..], reg_idx); + NativeEndian::write_u64(&mut buf[8..], offset); + NativeEndian::write_u32(&mut buf[16..], data.len() as u32); + buf[20..20 + data.len()].clone_from_slice(data); + handle_eintr!(self.sock.send(&buf)) + .map(|_| ()) + .map_err(Error::Io) + } + + fn recv_resp(&self, data: &mut [u8]) -> Result<()> { + let mut buf = [0; MSG_SIZE]; + handle_eintr!(self.sock.recv(&mut buf)).map_err(Error::Io)?; + let len = data.len(); + data.clone_from_slice(&buf[16..16 + len]); + Ok(()) + } + + fn wait(&self) -> Result<()> { + let mut buf = [0; MSG_SIZE]; + handle_eintr!(self.sock.recv(&mut buf)) + .map(|_| ()) + .map_err(Error::Io) } } impl BusDevice for ProxyDevice { fn config_register_write(&mut self, reg_idx: usize, offset: u64, data: &[u8]) { - let len = data.len() as u32; - let mut buffer = [0u8; 4]; - buffer[0..data.len()].clone_from_slice(data); - let reg_idx = reg_idx as u32; - let offset = offset as u32; - self.sync_send(Command::WriteConfig { - reg_idx, - offset, - len, - data: buffer, - }); - } - - fn config_register_read(&self, reg_idx: usize) -> u32 { - let res = self.sync_send(Command::ReadConfig(reg_idx as u32)); - if let Some(CommandResult::ReadConfigResult(val)) = res { - val - } else { - 0 + let res = self + .send_config_cmd(Command::WriteConfig, reg_idx as u32, offset, data) + .and_then(|_| self.wait()); + if let Err(e) = res { + error!("failed write to child device process: {}", e); } } + fn config_register_read(&self, reg_idx: usize) -> u32 { + let mut data = [0u8; 4]; + let res = self + .send_config_cmd(Command::ReadConfig, reg_idx as u32, 0, &[]) + .and_then(|_| self.recv_resp(&mut data)); + if let Err(e) = res { + error!("failed write to child device process: {}", e); + } + LittleEndian::read_u32(&data) + } + fn read(&mut self, offset: u64, data: &mut [u8]) { - let len = data.len() as u32; - if let Some(CommandResult::ReadResult(buffer)) = - self.sync_send(Command::Read { len, offset }) - { - let len = data.len(); - data.clone_from_slice(&buffer[0..len]); + let res = self + .send_cmd(Command::Read, offset, data.len() as u32, &[]) + .and_then(|_| self.recv_resp(data)); + if let Err(e) = res { + error!("failed read from child device process: {}", e); } } fn write(&mut self, offset: u64, data: &[u8]) { - let mut buffer = [0u8; 8]; - let len = data.len() as u32; - buffer[0..data.len()].clone_from_slice(data); - self.sync_send(Command::Write { - len, - offset, - data: buffer, - }); + let res = self + .send_cmd(Command::Write, offset, data.len() as u32, data) + .and_then(|_| self.wait()); + if let Err(e) = res { + error!("failed write to child device process: {}", e); + } } } impl Drop for ProxyDevice { fn drop(&mut self) { - self.sync_send(Command::Shutdown); + let res = self.send_cmd(Command::Shutdown, 0, 0, &[]); + if let Err(e) = res { + error!("failed to shutdown child device process: {}", e); + } } }