From 0b2111b029f90b7045b9a18e5d035c32a049bffd Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 21 Dec 2024 14:36:11 +0100 Subject: [PATCH 1/6] Port inputd to redox-scheme --- Cargo.lock | 1 + inputd/Cargo.toml | 1 + inputd/src/main.rs | 61 +++++++++++++++++++++++++++++----------------- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6954ea031f..a9a546933b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -677,6 +677,7 @@ dependencies = [ "log", "orbclient", "redox-daemon", + "redox-scheme", "redox_syscall", "spin", ] diff --git a/inputd/Cargo.toml b/inputd/Cargo.toml index 0ba28069f3..fa8ae45fce 100644 --- a/inputd/Cargo.toml +++ b/inputd/Cargo.toml @@ -14,3 +14,4 @@ spin = "0.9.8" libredox = "0.1.3" common = { path = "../common" } +redox-scheme = { git = "https://gitlab.redox-os.org/redox-os/redox-scheme.git" } diff --git a/inputd/src/main.rs b/inputd/src/main.rs index be8ae15f6b..446a68af7a 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -13,17 +13,17 @@ use std::collections::BTreeMap; use std::fs::File; -use std::io::{Read, Write}; use std::os::fd::AsRawFd; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use inputd::{Cmd, VtActivate}; +use redox_scheme::{RequestKind, SchemeMut, SignalBehavior, Socket, V2}; use spin::Mutex; use orbclient::{Event, EventOption}; -use syscall::{Error as SysError, EventFlags, Packet, SchemeMut, EINVAL}; +use syscall::{Error as SysError, EventFlags, EINVAL}; enum Handle { Producer, @@ -89,6 +89,8 @@ struct InputScheme { active_vt: Option>, todo: Vec, + + has_new_events: bool, } impl InputScheme { @@ -103,6 +105,8 @@ impl InputScheme { active_vt: None, todo: vec![], + + has_new_events: false, } } } @@ -162,7 +166,13 @@ impl SchemeMut for InputScheme { } } - fn read(&mut self, id: usize, buf: &mut [u8]) -> syscall::Result { + fn read( + &mut self, + id: usize, + buf: &mut [u8], + _offset: u64, + _fcntl_flags: u32, + ) -> syscall::Result { let handle = self.handles.get_mut(&id).ok_or(SysError::new(EINVAL))?; match handle { @@ -188,7 +198,15 @@ impl SchemeMut for InputScheme { } } - fn write(&mut self, id: usize, buf: &[u8]) -> syscall::Result { + fn write( + &mut self, + id: usize, + buf: &[u8], + _offset: u64, + _fcntl_flags: u32, + ) -> syscall::Result { + self.has_new_events = true; + let handle = self.handles.get_mut(&id).ok_or(SysError::new(EINVAL))?; match handle { @@ -351,24 +369,29 @@ impl SchemeMut for InputScheme { fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { // Create the ":input" scheme. - let mut socket_file = File::create(":input")?; + let socket_file: Socket = Socket::create("input")?; let mut scheme = InputScheme::new(); deamon.ready().unwrap(); loop { - let mut should_handle = false; - let mut packet = Packet::default(); - socket_file.read(&mut packet)?; + scheme.has_new_events = false; + let Some(request) = socket_file.next_request(SignalBehavior::Restart)? else { + // Scheme likely got unmounted + return Ok(()); + }; - // The producer has written to the channel; the consumers should be notified. - if packet.a == syscall::SYS_WRITE { - should_handle = true; + match request.kind() { + RequestKind::Call(call_request) => { + socket_file.write_response( + call_request.handle_scheme_mut(&mut scheme), + SignalBehavior::Restart, + )?; + } + RequestKind::Cancellation(_cancellation_request) => {}, + RequestKind::MsyncMsg | RequestKind::MunmapMsg | RequestKind::MmapMsg => unreachable!(), } - scheme.handle(&mut packet); - socket_file.write(&packet)?; - while let Some(cmd) = scheme.todo.pop() { let vt = scheme.vts.get_mut(&cmd.vt).unwrap(); let mut vt_inner = vt.inner().lock(); @@ -383,7 +406,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { scheme.active_vt = Some(vt.clone()); } - if !should_handle { + if !scheme.has_new_events { continue; } @@ -408,13 +431,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { } // Notify the consumer that we have some events to read. Yum yum. - let mut event_packet = Packet::default(); - event_packet.a = syscall::SYS_FEVENT; - event_packet.b = *id; - event_packet.c = EventFlags::EVENT_READ.bits(); - // Specifies the number of bytes that can be read non-blocking. - event_packet.d = pending.len(); - socket_file.write(&event_packet)?; + socket_file.post_fevent(*id, EventFlags::EVENT_READ.bits())?; *notified = true; } From f4193c688633bad33a6f9c1a62528de4256a868c Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 21 Dec 2024 15:38:13 +0100 Subject: [PATCH 2/6] inputd: Avoid panic when client tried to perform invalid operation --- inputd/src/main.rs | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/inputd/src/main.rs b/inputd/src/main.rs index 446a68af7a..69f7c9b7a6 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -194,7 +194,10 @@ impl SchemeMut for InputScheme { Ok(vt) } - _ => unreachable!(), + Handle::Producer => { + log::error!("inputd: producer tried to read"); + return Err(SysError::new(EINVAL)); + } } } @@ -211,7 +214,10 @@ impl SchemeMut for InputScheme { match handle { Handle::Device { device } => { - assert!(buf.len() == core::mem::size_of::()); + if buf.len() != core::mem::size_of::() { + log::error!("inputd: device tried to write incorrectly sized command"); + return Err(SysError::new(EINVAL)); + } // SAFETY: We have verified the size of the buffer above. let cmd = unsafe { &*buf.as_ptr().cast::() }; @@ -222,8 +228,11 @@ impl SchemeMut for InputScheme { return Ok(buf.len()); } - Handle::Consumer { .. } => unreachable!(), - _ => {} + Handle::Consumer { .. } => { + log::error!("inputd: consumer tried to write"); + return Err(SysError::new(EINVAL)); + } + Handle::Producer => {} } if buf.len() == 1 && buf[0] > 0xf4 { @@ -355,11 +364,13 @@ impl SchemeMut for InputScheme { } => { *events = flags; *notified = false; + Ok(EventFlags::empty()) + } + Handle::Producer | Handle::Device { .. } => { + log::error!("inputd: producer or device tried to use an event queue"); + Err(SysError::new(EINVAL)) } - _ => unreachable!(), } - - Ok(EventFlags::empty()) } fn close(&mut self, _id: usize) -> syscall::Result { @@ -388,7 +399,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { SignalBehavior::Restart, )?; } - RequestKind::Cancellation(_cancellation_request) => {}, + RequestKind::Cancellation(_cancellation_request) => {} RequestKind::MsyncMsg | RequestKind::MunmapMsg | RequestKind::MmapMsg => unreachable!(), } From 6a1cb33105253828c25a028710370bc20253ff8d Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 21 Dec 2024 15:52:36 +0100 Subject: [PATCH 3/6] inputd: Misc changes --- inputd/src/main.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/inputd/src/main.rs b/inputd/src/main.rs index 69f7c9b7a6..0062e68b2b 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -59,7 +59,7 @@ struct Vt { } impl Vt { - pub fn new(display: D, index: usize) -> Arc + fn new(display: D, index: usize) -> Arc where D: Into, { @@ -70,7 +70,7 @@ impl Vt { }) } - pub fn inner(&self) -> &Mutex { + fn inner(&self) -> &Mutex { self.inner.call_once(|| { let handle_file = File::open(format!("/scheme/{}/handle", self.display)).unwrap(); Mutex::new(VtInner { handle_file }) @@ -88,13 +88,12 @@ struct InputScheme { super_key: bool, active_vt: Option>, - todo: Vec, - + pending_activate: Option, has_new_events: bool, } impl InputScheme { - pub fn new() -> Self { + fn new() -> Self { Self { next_id: AtomicUsize::new(0), next_vt_id: AtomicUsize::new(1), @@ -104,8 +103,7 @@ impl InputScheme { super_key: false, active_vt: None, - todo: vec![], - + pending_activate: None, has_new_events: false, } } @@ -223,7 +221,7 @@ impl SchemeMut for InputScheme { let cmd = unsafe { &*buf.as_ptr().cast::() }; self.vts.insert(cmd.vt, Vt::new(device.clone(), cmd.vt)); - self.todo.push(cmd.clone()); + self.pending_activate = Some(cmd.clone()); return Ok(buf.len()); } @@ -403,7 +401,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { RequestKind::MsyncMsg | RequestKind::MunmapMsg | RequestKind::MmapMsg => unreachable!(), } - while let Some(cmd) = scheme.todo.pop() { + if let Some(cmd) = scheme.pending_activate.take() { let vt = scheme.vts.get_mut(&cmd.vt).unwrap(); let mut vt_inner = vt.inner().lock(); @@ -455,7 +453,7 @@ fn daemon_runner(redox_daemon: redox_daemon::Daemon) -> ! { unreachable!(); } -pub fn main() { +fn main() { common::setup_logging( "misc", "inputd", From 08f619bc229e954a0bb331121bbf8e9677bf7830 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 21 Dec 2024 15:58:07 +0100 Subject: [PATCH 4/6] inputd: Remove usage of Arc for Vt Storing the vt index instead of a direct reference to the vt is just as easy. --- inputd/src/main.rs | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/inputd/src/main.rs b/inputd/src/main.rs index 0062e68b2b..928b8531e8 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -15,7 +15,6 @@ use std::collections::BTreeMap; use std::fs::File; use std::os::fd::AsRawFd; use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::Arc; use inputd::{Cmd, VtActivate}; @@ -59,15 +58,12 @@ struct Vt { } impl Vt { - fn new(display: D, index: usize) -> Arc - where - D: Into, - { - Arc::new(Self { + fn new(display: impl Into, index: usize) -> Self { + Self { display: display.into(), inner: spin::Once::new(), index, - }) + } } fn inner(&self) -> &Mutex { @@ -84,9 +80,9 @@ struct InputScheme { next_id: AtomicUsize, next_vt_id: AtomicUsize, - vts: BTreeMap>, + vts: BTreeMap, super_key: bool, - active_vt: Option>, + active_vt: Option, pending_activate: Option, has_new_events: bool, @@ -272,7 +268,7 @@ impl SchemeMut for InputScheme { }, EventOption::Resize(resize_event) => { - let active_vt = self.active_vt.as_ref().unwrap(); + let active_vt = &self.vts[&self.active_vt.unwrap()]; let mut vt_inner = active_vt.inner().lock(); inputd::send_comand( @@ -292,13 +288,13 @@ impl SchemeMut for InputScheme { } if let Some(new_active) = new_active_opt { - if new_active == self.active_vt.as_ref().unwrap().index { + if new_active == self.vts[&self.active_vt.unwrap()].index { continue 'out; } - if let Some(new_active) = self.vts.get(&new_active).cloned() { + if let Some(new_active) = self.vts.get(&new_active) { { - let active_vt = self.active_vt.as_ref().unwrap(); + let active_vt = &self.vts[&self.active_vt.unwrap()]; let mut vt_inner = active_vt.inner().lock(); inputd::send_comand( @@ -315,7 +311,7 @@ impl SchemeMut for InputScheme { vt: new_active.index, }, )?; - self.active_vt = Some(new_active.clone()); + self.active_vt = Some(new_active.index); } else { log::warn!("inputd: switch to non-existent VT #{new_active} was requested"); } @@ -324,7 +320,7 @@ impl SchemeMut for InputScheme { assert!(handle.is_producer()); - let active_vt = self.active_vt.as_ref().unwrap(); + let active_vt = self.active_vt.unwrap(); for handle in self.handles.values_mut() { match handle { Handle::Consumer { @@ -333,7 +329,7 @@ impl SchemeMut for InputScheme { vt, .. } => { - if *vt != active_vt.index { + if *vt != active_vt { continue; } @@ -412,7 +408,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { log::error!("inputd: failed to activate VT #{}: {err}", vt.index) } - scheme.active_vt = Some(vt.clone()); + scheme.active_vt = Some(vt.index); } if !scheme.has_new_events { @@ -431,11 +427,11 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { continue; } - let active_vt = scheme.active_vt.as_ref().unwrap(); + let active_vt = scheme.active_vt.unwrap(); // The activate VT is not the same as the VT that the consumer is listening to // for events. - if active_vt.index != *vt { + if active_vt != *vt { continue; } From cf84e712200f1586cb9c30f00c3827530c94a26d Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 21 Dec 2024 16:00:34 +0100 Subject: [PATCH 5/6] inputd: Use std's concurrency primitives instead of spin --- Cargo.lock | 1 - inputd/Cargo.toml | 1 - inputd/src/main.rs | 17 +++++++++-------- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a9a546933b..27fb0ab0b9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -679,7 +679,6 @@ dependencies = [ "redox-daemon", "redox-scheme", "redox_syscall", - "spin", ] [[package]] diff --git a/inputd/Cargo.toml b/inputd/Cargo.toml index fa8ae45fce..d45b055cb1 100644 --- a/inputd/Cargo.toml +++ b/inputd/Cargo.toml @@ -10,7 +10,6 @@ log = "0.4.19" redox-daemon = "0.1.2" redox_syscall = "0.5" orbclient = "0.3.27" -spin = "0.9.8" libredox = "0.1.3" common = { path = "../common" } diff --git a/inputd/src/main.rs b/inputd/src/main.rs index 928b8531e8..78b97f7f1a 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -11,15 +11,16 @@ //! Read events from `input:consumer`. Optionally, set the `EVENT_READ` flag to be notified when //! events are available. +use std::cell::OnceCell; use std::collections::BTreeMap; use std::fs::File; use std::os::fd::AsRawFd; use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Mutex; use inputd::{Cmd, VtActivate}; use redox_scheme::{RequestKind, SchemeMut, SignalBehavior, Socket, V2}; -use spin::Mutex; use orbclient::{Event, EventOption}; use syscall::{Error as SysError, EventFlags, EINVAL}; @@ -54,20 +55,20 @@ struct VtInner { struct Vt { display: String, index: usize, - inner: spin::Once>, + inner: OnceCell>, } impl Vt { fn new(display: impl Into, index: usize) -> Self { Self { display: display.into(), - inner: spin::Once::new(), + inner: OnceCell::new(), index, } } fn inner(&self) -> &Mutex { - self.inner.call_once(|| { + self.inner.get_or_init(|| { let handle_file = File::open(format!("/scheme/{}/handle", self.display)).unwrap(); Mutex::new(VtInner { handle_file }) }) @@ -269,7 +270,7 @@ impl SchemeMut for InputScheme { EventOption::Resize(resize_event) => { let active_vt = &self.vts[&self.active_vt.unwrap()]; - let mut vt_inner = active_vt.inner().lock(); + let mut vt_inner = active_vt.inner().lock().unwrap(); inputd::send_comand( &mut vt_inner.handle_file, @@ -295,7 +296,7 @@ impl SchemeMut for InputScheme { if let Some(new_active) = self.vts.get(&new_active) { { let active_vt = &self.vts[&self.active_vt.unwrap()]; - let mut vt_inner = active_vt.inner().lock(); + let mut vt_inner = active_vt.inner().lock().unwrap(); inputd::send_comand( &mut vt_inner.handle_file, @@ -303,7 +304,7 @@ impl SchemeMut for InputScheme { )?; } - let mut vt_inner = new_active.inner().lock(); + let mut vt_inner = new_active.inner().lock().unwrap(); inputd::send_comand( &mut vt_inner.handle_file, @@ -399,7 +400,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { if let Some(cmd) = scheme.pending_activate.take() { let vt = scheme.vts.get_mut(&cmd.vt).unwrap(); - let mut vt_inner = vt.inner().lock(); + let mut vt_inner = vt.inner().lock().unwrap(); // Failing to activate a VT is not a fatal error. if let Err(err) = From 3c00151d49db65fc9f4b70143ff44e3a59197dea Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 21 Dec 2024 16:15:19 +0100 Subject: [PATCH 6/6] inputd: Remove the vt handle mutex --- inputd/src/main.rs | 79 ++++++++++++++++------------------------------ 1 file changed, 27 insertions(+), 52 deletions(-) diff --git a/inputd/src/main.rs b/inputd/src/main.rs index 78b97f7f1a..8aa562a888 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -11,12 +11,10 @@ //! Read events from `input:consumer`. Optionally, set the `EVENT_READ` flag to be notified when //! events are available. -use std::cell::OnceCell; use std::collections::BTreeMap; use std::fs::File; use std::os::fd::AsRawFd; use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::Mutex; use inputd::{Cmd, VtActivate}; @@ -44,34 +42,29 @@ impl Handle { } } -/// VT Inner State -/// -/// This is *required* to be lazily initialized since opening the handle to the display -/// requires the system call to return first. Otherwise, it will block indefinitely. -struct VtInner { - handle_file: File, -} - struct Vt { display: String, index: usize, - inner: OnceCell>, + + /// This is *required* to be lazily initialized since opening the handle to the display + /// requires the system call to return first. Otherwise, it will block indefinitely. + handle_file: Option, } impl Vt { fn new(display: impl Into, index: usize) -> Self { Self { display: display.into(), - inner: OnceCell::new(), + handle_file: None, index, } } - fn inner(&self) -> &Mutex { - self.inner.get_or_init(|| { - let handle_file = File::open(format!("/scheme/{}/handle", self.display)).unwrap(); - Mutex::new(VtInner { handle_file }) - }) + fn send_command(&mut self, cmd: Cmd) -> Result<(), libredox::error::Error> { + let handle_file = self + .handle_file + .get_or_insert_with(|| File::open(format!("/scheme/{}/handle", self.display)).unwrap()); + inputd::send_comand(handle_file, cmd) } } @@ -269,20 +262,15 @@ impl SchemeMut for InputScheme { }, EventOption::Resize(resize_event) => { - let active_vt = &self.vts[&self.active_vt.unwrap()]; - let mut vt_inner = active_vt.inner().lock().unwrap(); + let active_vt = self.vts.get_mut(&self.active_vt.unwrap()).unwrap(); + active_vt.send_command(Cmd::Resize { + vt: active_vt.index, + width: resize_event.width, + height: resize_event.height, - inputd::send_comand( - &mut vt_inner.handle_file, - Cmd::Resize { - vt: active_vt.index, - width: resize_event.width, - height: resize_event.height, - - // TODO(andypython): Figure out how to get the stride. - stride: resize_event.width, - }, - )?; + // TODO(andypython): Figure out how to get the stride. + stride: resize_event.width, + })?; } _ => continue, @@ -293,25 +281,16 @@ impl SchemeMut for InputScheme { continue 'out; } - if let Some(new_active) = self.vts.get(&new_active) { - { - let active_vt = &self.vts[&self.active_vt.unwrap()]; - let mut vt_inner = active_vt.inner().lock().unwrap(); + if self.vts.contains_key(&new_active) { + let active_vt = self.vts.get_mut(&self.active_vt.unwrap()).unwrap(); - inputd::send_comand( - &mut vt_inner.handle_file, - Cmd::Deactivate(active_vt.index), - )?; - } + active_vt.send_command(Cmd::Deactivate(active_vt.index))?; + } - let mut vt_inner = new_active.inner().lock().unwrap(); - - inputd::send_comand( - &mut vt_inner.handle_file, - Cmd::Activate { - vt: new_active.index, - }, - )?; + if let Some(new_active) = self.vts.get_mut(&new_active) { + new_active.send_command(Cmd::Activate { + vt: new_active.index, + })?; self.active_vt = Some(new_active.index); } else { log::warn!("inputd: switch to non-existent VT #{new_active} was requested"); @@ -400,12 +379,8 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { if let Some(cmd) = scheme.pending_activate.take() { let vt = scheme.vts.get_mut(&cmd.vt).unwrap(); - let mut vt_inner = vt.inner().lock().unwrap(); - // Failing to activate a VT is not a fatal error. - if let Err(err) = - inputd::send_comand(&mut vt_inner.handle_file, Cmd::Activate { vt: vt.index }) - { + if let Err(err) = vt.send_command(Cmd::Activate { vt: vt.index }) { log::error!("inputd: failed to activate VT #{}: {err}", vt.index) }