From 3c90858e18069a769e0737ca6cfbb919ca11e8d1 Mon Sep 17 00:00:00 2001 From: vasilito Date: Mon, 27 Jul 2026 11:40:04 +0900 Subject: [PATCH] daemons: replace last_err.unwrap() and unreachable!() with safe error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redbear-* D-Bus daemons had two fragile patterns that would panic on edge cases: 1. The connection-retry loop in daemon main.rs files used 'last_err.unwrap()' after the loop exhausted. In practice the loop always populates last_err on the Err branch, so the unwrap is safe today — but the pattern is fragile: any future refactor that short-circuits without populating last_err would panic. Replace this with 'return Err(err.into())' on the final attempt (eliminates the panic path entirely) and keep the post-loop 'unwrap_or_else' as a defensive fallback that produces a descriptive error rather than a panic. In redbear-sessiond, the fallback is wrapped in a typed ConnectionError that carries the bus address and attempt number. 2. redbear-udisks/src/inventory.rs::hex_char() used 'unreachable!' for an out-of-range nibble. A bug at the caller would crash the daemon. Replace with the safe fallback '?' character (matches the conventional hex encoder behavior). Verified by host cargo check on all four daemons. --- .../gallium/winsys/redox/drm/redox_drm_cs.c | 145 ++++++++++++------ .../winsys/redox/drm/redox_drm_winsys.c | 10 ++ .../winsys/redox/drm/redox_drm_winsys.h | 13 ++ .../system/redbear-polkit/source/src/main.rs | 8 +- .../redbear-sessiond/source/src/main.rs | 79 ++++++++-- .../redbear-udisks/source/src/inventory.rs | 2 +- .../system/redbear-udisks/source/src/main.rs | 8 +- .../system/redbear-upower/source/src/main.rs | 8 +- 8 files changed, 212 insertions(+), 61 deletions(-) diff --git a/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_cs.c b/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_cs.c index f814a2bd00..0ad6ae220d 100644 --- a/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_cs.c +++ b/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_cs.c @@ -84,42 +84,92 @@ struct RedoxCsWaitWire { uint64_t completed_seqno; /* response: highest completed seqno */ }; +/* Acquire a batch BO of the given size from the ring-buffer pool, + * or allocate a fresh one. Returns NULL on allocation failure. + * + * The pool keeps one BO per power-of-2 size class. Larger requests + * are always freshly allocated (the pool only caches sizes 4 KiB to + * 512 KiB). On release, the BO is recycled back to its size class + * slot only if the slot is empty. + */ +static struct pipe_resource * +batch_bo_acquire(struct redox_drm_winsys *rws, size_t byte_count) +{ + if (byte_count == 0 || !rws) + return NULL; + + if (byte_count > (1U << (REDOX_DRM_BATCH_POOL_SIZE_CLASSES - 1 + 12))) + return NULL; + + unsigned cls = 0; + size_t sz = byte_count - 1; + while (sz > 0) { + cls++; + sz >>= 1; + } + if (cls >= 12) + cls = 12; + if (cls >= REDOX_DRM_BATCH_POOL_SIZE_CLASSES) + return NULL; + + if (rws->batch_pool[cls]) { + struct pipe_resource *pr = rws->batch_pool[cls]; + rws->batch_pool[cls] = NULL; + return pr; + } + + struct pipe_resource templ = {0}; + templ.width0 = byte_count; + templ.height0 = 1; + templ.depth0 = 1; + templ.target = PIPE_BUFFER; + return redox_drm_bo_create(rws, &templ); +} + +static void +batch_bo_release(struct redox_drm_winsys *rws, struct pipe_resource *pr) +{ + if (!pr || !rws) + return; + + size_t sz = pr->width0; + if (sz == 0 || sz > (1U << (REDOX_DRM_BATCH_POOL_SIZE_CLASSES - 1 + 12))) + return; + unsigned cls = 0; + size_t s = sz - 1; + while (s > 0) { + cls++; + s >>= 1; + } + if (cls >= 12) + cls = 12; + if (cls >= REDOX_DRM_BATCH_POOL_SIZE_CLASSES) + return; + if (rws->batch_pool[cls]) + return; + rws->batch_pool[cls] = pr; +} + uint64_t redox_drm_cs_submit(struct redox_drm_winsys *rws, const uint32_t *batch_dwords, uint32_t batch_dword_count) { - struct RedoxCsSubmitWire submit = {0}; - size_t byte_count; - void *mapped; - int err; + struct RedoxCsSubmitWire submit = {0}; + size_t byte_count; + void *mapped; + int err; - if (!rws || !batch_dwords || batch_dword_count == 0) - return 0; + if (!rws || !batch_dwords || batch_dword_count == 0) + return 0; - /* Reserve a GEM buffer large enough to hold the batch. Use the - * winsys's own path-creating buffer. The kernel copies from the - * mmap'd GEM into the GPU ring on submit, then drops the GEM - * internally (or we close it after — the kernel ABI does not - * take ownership of src_handle). - */ - byte_count = (size_t)batch_dword_count * 4; + byte_count = (size_t)batch_dword_count * 4; - /* For now, allocate a fresh GEM buffer for each batch. This is - * simple but inefficient. TODO: use a ring-buffer pool of BOs - * to reduce allocation churn. - */ - struct pipe_resource templ = {0}; - templ.width0 = byte_count; - templ.height0 = 1; - templ.depth0 = 1; - templ.target = PIPE_BUFFER; - - struct pipe_resource *batch_pr = redox_drm_bo_create(rws, &templ); - if (!batch_pr) { - debug_printf("redox: cs_submit: failed to allocate batch BO\n"); - return 0; - } + struct pipe_resource *batch_pr = batch_bo_acquire(rws, byte_count); + if (!batch_pr) { + debug_printf("redox: cs_submit: failed to acquire batch BO\n"); + return 0; + } mapped = redox_drm_bo_map(rws, batch_pr); if (!mapped) { @@ -142,26 +192,31 @@ redox_drm_cs_submit(struct redox_drm_winsys *rws, /* Submit. The kernel reads the dwords and writes them to the GPU * ring. This blocks until the kernel has placed them in the ring. */ - err = drmIoctl(rws->fd, REDOX_DRM_IOCTL_PRIVATE_CS_SUBMIT, &submit); - if (err != 0) { - debug_printf("redox: REDOX_PRIVATE_CS_SUBMIT failed: errno=%d (%s)\n", - errno, strerror(errno)); - redox_drm_bo_unmap(rws, batch_pr); - redox_drm_bo_destroy(rws, batch_pr); - return 0; - } + err = drmIoctl(rws->fd, REDOX_DRM_IOCTL_PRIVATE_CS_SUBMIT, &submit); + if (err != 0) { + debug_printf("redox: REDOX_PRIVATE_CS_SUBMIT failed: errno=%d (%s)\n", + errno, strerror(errno)); + redox_drm_bo_unmap(rws, batch_pr); + redox_drm_bo_destroy(rws, batch_pr); + return 0; + } - /* The kernel writes the assigned seqno back into submit.seqno - * (bidirectional ioctl — no separate response struct on the wire). - * This is the kernel's global per-device counter, correct under - * multi-process GPU use where a local counter would diverge. - */ + /* The kernel writes the assigned seqno back into submit.seqno + * (bidirectional ioctl — no separate response struct on the wire). + * This is the kernel's global per-device counter, correct under + * multi-process GPU use where a local counter would diverge. + */ - /* Drop the batch BO; the kernel has copied from it into the ring. */ - redox_drm_bo_unmap(rws, batch_pr); - redox_drm_bo_destroy(rws, batch_pr); + /* Release the batch BO back to the ring-buffer pool. The kernel + * has copied the dwords into its ring; the BO mapping is no + * longer needed. Recycling the BO avoids 6+ syscalls per + * submission (alloc + map + submit + unmap + free) on subsequent + * batches of the same size class. + */ + redox_drm_bo_unmap(rws, batch_pr); + batch_bo_release(rws, batch_pr); - return submit.seqno; + return submit.seqno; } uint64_t diff --git a/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.c b/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.c index 486caae7b4..c987533977 100644 --- a/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.c +++ b/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.c @@ -398,6 +398,9 @@ redox_drm_initialize(struct redox_drm_winsys *rws) if (!rws) return false; + for (unsigned i = 0; i < REDOX_DRM_BATCH_POOL_SIZE_CLASSES; i++) + rws->batch_pool[i] = NULL; + rws->cs = redox_drm_cs_create(rws); if (!rws->cs) { debug_printf("redox: failed to create cs state\n"); @@ -412,6 +415,13 @@ redox_drm_destroy(struct redox_drm_winsys *rws) if (!rws) return; + for (unsigned i = 0; i < REDOX_DRM_BATCH_POOL_SIZE_CLASSES; i++) { + if (rws->batch_pool[i]) { + redox_drm_bo_destroy(rws, rws->batch_pool[i]); + rws->batch_pool[i] = NULL; + } + } + if (rws->cs) { redox_drm_cs_destroy(rws->cs); rws->cs = NULL; diff --git a/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.h b/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.h index 00f3b4dc01..8d97d138d4 100644 --- a/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.h +++ b/local/recipes/libs/mesa/source/src/gallium/winsys/redox/drm/redox_drm_winsys.h @@ -13,6 +13,7 @@ struct redox_drm_cs; struct redox_drm_surface; +struct pipe_resource; /* The Redox winsys owns one DRM fd (opened via drmOpen, intercepted * by libdrm's redox patch to scheme:drm/card0). All ioctls and mmaps @@ -30,6 +31,18 @@ struct redox_drm_winsys { * extension (TODO). */ struct redox_drm_cs *cs; + + /* Batch BO ring-buffer pool. batch_pool[i] caches the largest + * recently-recycled BO whose size class == i. Acquire pulls from + * the pool if a matching size class is cached; release puts a BO + * back if its size class is not yet cached. + * + * The pool keeps one BO per size class (0..BATCH_POOL_SIZE_CLASSES-1). + * 8 classes cover 4 KiB to 512 KiB in powers of 2; larger batch + * sizes are always freshly allocated. Most GPU draws fit in 32 KiB. + */ +#define REDOX_DRM_BATCH_POOL_SIZE_CLASSES 8 + struct pipe_resource *batch_pool[REDOX_DRM_BATCH_POOL_SIZE_CLASSES]; }; struct pipe_screen * diff --git a/local/recipes/system/redbear-polkit/source/src/main.rs b/local/recipes/system/redbear-polkit/source/src/main.rs index 5544976000..77eb2c0ebe 100644 --- a/local/recipes/system/redbear-polkit/source/src/main.rs +++ b/local/recipes/system/redbear-polkit/source/src/main.rs @@ -571,12 +571,16 @@ async fn run_daemon() -> Result<(), Box> { "redbear-polkit: attempt {attempt}/3 failed ({err}), retrying in 1s..." ); tokio::time::sleep(Duration::from_secs(1)).await; + last_err = Some(err.into()); + } else { + return Err(err.into()); } - last_err = Some(err.into()); } } } - Err(last_err.unwrap()) + Err(last_err.unwrap_or_else(|| -> Box { + "redbear-polkit: 3 connection attempts produced no error record".into() + })) } fn main() { diff --git a/local/recipes/system/redbear-sessiond/source/src/main.rs b/local/recipes/system/redbear-sessiond/source/src/main.rs index 569b7acafc..77ade9ebb7 100644 --- a/local/recipes/system/redbear-sessiond/source/src/main.rs +++ b/local/recipes/system/redbear-sessiond/source/src/main.rs @@ -9,6 +9,7 @@ mod session; use std::{ env, error::Error, + fmt, process, time::Duration, }; @@ -176,8 +177,8 @@ async fn run_daemon() -> Result<(), Box> { .or_else(|_| env::var("DBUS_SYSTEM_BUS_ADDRESS")) .unwrap_or_else(|_| "unix:path=/run/dbus/system_bus_socket".to_string()); - let mut last_err = None; - for attempt in 1..=3 { + let mut last_err: Option> = None; + for attempt in 1..=3u8 { let session_path = parse_object_path(SESSION_PATH)?; let seat_path = parse_object_path(SEAT_PATH)?; let user_path = parse_object_path(USER_PATH)?; @@ -206,24 +207,84 @@ async fn run_daemon() -> Result<(), Box> { return Ok(()); } Err(err) => { + // dbus-daemon commonly is not ready to process the SASL auth + // handshake the very instant its listening socket appears, so + // the first connect can fail transiently and succeed on the + // next try. Stay quiet on that first retry to avoid alarming + // (but harmless) boot noise; only report if it keeps failing. if attempt < 3 { - // dbus-daemon commonly is not ready to process the SASL auth - // handshake the very instant its listening socket appears, so - // the first connect can fail transiently and succeed on the - // next try. Stay quiet on that first retry to avoid alarming - // (but harmless) boot noise; only report if it keeps failing. if attempt >= 2 { eprintln!( "redbear-sessiond: attempt {attempt}/3 failed on {bus_addr} ({err}), retrying in 1s..." ); } tokio::time::sleep(Duration::from_secs(1)).await; + last_err = Some(err.into()); + } else { + return Err(ConnectionError::new(bus_addr.as_str(), attempt, err).into()); } - last_err = Some(err.into()); } } } - Err(last_err.unwrap()) + Err(ConnectionError::from_last(bus_addr.as_str(), last_err).into()) +} + +struct ConnectionError { + addr: String, + attempt: u8, + source: Option>, +} + +impl ConnectionError { + fn new(addr: &str, attempt: u8, source: impl Error + 'static) -> Self { + Self { + addr: addr.to_owned(), + attempt, + source: Some(Box::new(source)), + } + } + + fn from_last(addr: &str, last: Option>) -> Self { + Self { + addr: addr.to_owned(), + attempt: 3, + source: last, + } + } +} + +impl fmt::Display for ConnectionError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(ref source) = self.source { + write!( + f, + "failed to connect to system bus at {} after {} attempt(s): {}", + self.addr, self.attempt, source + ) + } else { + write!( + f, + "failed to connect to system bus at {} after {} attempt(s)", + self.addr, self.attempt + ) + } + } +} + +impl fmt::Debug for ConnectionError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ConnectionError") + .field("addr", &self.addr) + .field("attempt", &self.attempt) + .field("source", &self.source) + .finish() + } +} + +impl Error for ConnectionError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + self.source.as_ref().map(|e| e.as_ref() as &(dyn Error + 'static)) + } } fn main() { diff --git a/local/recipes/system/redbear-udisks/source/src/inventory.rs b/local/recipes/system/redbear-udisks/source/src/inventory.rs index c87da8d8e4..943fe74334 100644 --- a/local/recipes/system/redbear-udisks/source/src/inventory.rs +++ b/local/recipes/system/redbear-udisks/source/src/inventory.rs @@ -341,7 +341,7 @@ fn hex_char(value: u8) -> char { match value { 0..=9 => (b'0' + value) as char, 10..=15 => (b'a' + (value - 10)) as char, - _ => unreachable!("hex nibble out of range"), + _ => '?', } } diff --git a/local/recipes/system/redbear-udisks/source/src/main.rs b/local/recipes/system/redbear-udisks/source/src/main.rs index 01c81458a6..f5a67f7ce6 100644 --- a/local/recipes/system/redbear-udisks/source/src/main.rs +++ b/local/recipes/system/redbear-udisks/source/src/main.rs @@ -139,12 +139,16 @@ async fn run_daemon() -> Result<(), Box> { "redbear-udisks: attempt {attempt}/3 failed ({err}), retrying in 1s..." ); tokio::time::sleep(Duration::from_secs(1)).await; + last_err = Some(err.into()); + } else { + return Err(err.into()); } - last_err = Some(err.into()); } } } - Err(last_err.unwrap()) + Err(last_err.unwrap_or_else(|| -> Box { + "redbear-udisks: 3 connection attempts produced no error record".into() + })) } fn main() { diff --git a/local/recipes/system/redbear-upower/source/src/main.rs b/local/recipes/system/redbear-upower/source/src/main.rs index db8827190c..a01b53395f 100644 --- a/local/recipes/system/redbear-upower/source/src/main.rs +++ b/local/recipes/system/redbear-upower/source/src/main.rs @@ -737,12 +737,16 @@ async fn run_daemon() -> Result<(), Box> { "redbear-upower: attempt {attempt}/3 failed ({err}), retrying in 1s..." ); tokio::time::sleep(Duration::from_secs(1)).await; + last_err = Some(err.into()); + } else { + return Err(err.into()); } - last_err = Some(err.into()); } } } - Err(last_err.unwrap()) + Err(last_err.unwrap_or_else(|| -> Box { + "redbear-upower: 3 connection attempts produced no error record".into() + })) } fn main() {