From 7d0603d96ac0af5cb80fe4f4ba0b9545610a02f0 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 29 Oct 2024 09:52:02 +0100 Subject: [PATCH] fix(coap): Apply check and clippy lints --- clippy.toml | 2 +- examples/coap/src/main.rs | 6 +-- src/lib/coapcore/src/oluru.rs | 1 + src/lib/coapcore/src/seccontext.rs | 58 +++++++++++++++------------- src/riot-rs-coap/src/lib.rs | 16 ++++---- src/riot-rs-coap/src/udp_nal/mod.rs | 36 ++++++++++++----- src/riot-rs-coap/src/udp_nal/util.rs | 30 +++++++++----- 7 files changed, 90 insertions(+), 59 deletions(-) diff --git a/clippy.toml b/clippy.toml index 03b57f019..1c33daa56 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,4 +1,4 @@ # Require SAFETY docs, as well as a few other lints, for private items check-private-items = true -doc-valid-idents = ["MHz", "GHz", "THz", ".."] +doc-valid-idents = ["CoAP", "MHz", "GHz", "THz", ".."] diff --git a/examples/coap/src/main.rs b/examples/coap/src/main.rs index a67e30f5d..95e00cb48 100644 --- a/examples/coap/src/main.rs +++ b/examples/coap/src/main.rs @@ -3,8 +3,6 @@ #![feature(impl_trait_in_assoc_type)] #![feature(used_with_arg)] -use riot_rs::{debug::log::*, network}; - // because coapcore depends on it temporarily extern crate alloc; use static_alloc::Bump; @@ -14,7 +12,7 @@ static A: Bump<[u8; 1 << 16]> = Bump::uninit(); #[riot_rs::task(autostart)] async fn coap_run() { - use coap_handler_implementations::{HandlerBuilder, ReportingHandlerBuilder}; + use coap_handler_implementations::HandlerBuilder; let log = None; let buffer = scroll_ring::Buffer::<512>::default(); @@ -54,7 +52,7 @@ async fn run_client_operations(mut stdout: impl core::fmt::Write) { // shame let addr = "10.42.0.1:1234"; - let demoserver = addr.clone().parse().unwrap(); + let demoserver = addr.parse().unwrap(); use coap_request::Stack; writeln!(stdout, "Sending GET to {}...", addr).unwrap(); diff --git a/src/lib/coapcore/src/oluru.rs b/src/lib/coapcore/src/oluru.rs index ed9320d78..11e1b9e3c 100644 --- a/src/lib/coapcore/src/oluru.rs +++ b/src/lib/coapcore/src/oluru.rs @@ -13,6 +13,7 @@ //! indicate numeric values (including of priorities, where "high" corresponds to "small" and "low" //! to "large"). #![forbid(unsafe_code)] +#![expect(clippy::indexing_slicing, reason = "module is scheduled for overhaul")] use arrayvec::ArrayVec; diff --git a/src/lib/coapcore/src/seccontext.rs b/src/lib/coapcore/src/seccontext.rs index b49140a1c..ad8ef2395 100644 --- a/src/lib/coapcore/src/seccontext.rs +++ b/src/lib/coapcore/src/seccontext.rs @@ -71,9 +71,9 @@ impl COwn { } } -impl Into for COwn { - fn into(self) -> lakers::ConnId { - lakers::ConnId::from_int_raw(self.0) +impl From for lakers::ConnId { + fn from(cown: COwn) -> Self { + lakers::ConnId::from_int_raw(cown.0) } } @@ -106,9 +106,9 @@ impl AifStaticRest { .is_some_and(|o| o.value() == b"stdout") && uri_path_options.next().is_none() { - return self.may_use_stdout; + self.may_use_stdout } else { - return true; + true } } } @@ -134,6 +134,10 @@ impl Default for SecContextState { } #[derive(Debug)] +#[expect( + clippy::large_enum_variant, + reason = "requiring more memory during connection setup is expected, but the complexity of an inhmogenous pool is currently impractical" +)] enum SecContextStage { Empty, @@ -479,22 +483,21 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler return Err(Own(CoAPError::method_not_allowed())); } - let first_byte = request + let (first_byte, edhoc_m1) = request .payload() - .get(0) + .split_first() .ok_or_else(CoAPError::bad_request)?; let starts_with_true = first_byte == &0xf5; if starts_with_true { info!("Processing incoming EDHOC message 1"); let message_1 = - &lakers::EdhocMessageBuffer::new_from_slice(&request.payload()[1..]) - .map_err(too_small)?; + &lakers::EdhocMessageBuffer::new_from_slice(edhoc_m1).map_err(too_small)?; let (responder, c_i, ead_1) = lakers::EdhocResponder::new( (self.crypto_factory)(), - &self.own_identity.1, - self.own_identity.0.clone(), + self.own_identity.1, + *self.own_identity.0, ) .process_message_1(message_1) .map_err(render_error)?; @@ -512,12 +515,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler .filter_map(|entry| entry.corresponding_cown()) // C_R does not only need to be unique, it also must not be identical // to C_I - .chain( - COwn::from_kid(c_i.as_slice()) - .as_slice() - .into_iter() - .cloned(), - ), + .chain(COwn::from_kid(c_i.as_slice()).as_slice().iter().cloned()), ); debug!("Entries in pool:"); @@ -559,10 +557,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler // isn't processable, it's unlikely that another one would come up and be. let mut taken = self .pool - .lookup( - |c| c.corresponding_cown() == Some(kid), - |matched| core::mem::replace(matched, Default::default()), - ) + .lookup(|c| c.corresponding_cown() == Some(kid), core::mem::take) // following RFC8613 Section 8.2 item 2.2 // FIXME unauthorized (unreleased in coap-message-utils) .ok_or_else(CoAPError::bad_request)?; @@ -589,6 +584,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler } = taken { debug_assert_eq!(c_r, kid, "State was looked up by KID"); + #[allow(clippy::indexing_slicing, reason = "slice fits by construction")] let msg_3 = lakers::EdhocMessageBuffer::new_from_slice(&payload[..cutoff]) .map_err(|e| Own(too_small(e)))?; @@ -653,7 +649,13 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler let oscore_salt = responder.edhoc_exporter(1u8, &[], 8); // label is 1 let oscore_secret = &oscore_secret[..16]; let oscore_salt = &oscore_salt[..8]; - debug!("OSCORE secret: {:?}...", &oscore_secret[..5]); + #[allow( + clippy::indexing_slicing, + reason = "secret necessarily contains more than 40 bits" + )] + { + debug!("OSCORE secret: {:?}...", &oscore_secret[..5]); + } debug!("OSCORE salt: {:?}", &oscore_salt); let sender_id = c_i.as_slice(); @@ -665,8 +667,8 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler let immutables = liboscore::PrimitiveImmutables::derive( hkdf, - &oscore_secret, - &oscore_salt, + oscore_secret, + oscore_salt, None, aead, sender_id, @@ -740,6 +742,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler let oscore_option = oscore_option.unwrap(); let oscore_option = liboscore::OscoreOption::parse(&oscore_option) .map_err(|_| CoAPError::bad_option(coap_numbers::option::OSCORE))?; + #[allow(clippy::indexing_slicing, reason = "slice fits by construction")] allocated_message .set_payload(&payload[front_trim_payload..]) .unwrap(); @@ -795,7 +798,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler ) -> Result<(), Self::BuildResponseError> { use OrInner::{Inner, Own}; - Ok(match req { + match req { Own(EdhocResponse::OkSend2(c_r)) => { // FIXME: Why does the From not do the map_err? response.set_code( @@ -807,7 +810,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler |matched| { // temporary default will not live long (and may be only constructed if // prepare_message_2 fails) - let taken = core::mem::replace(matched, Default::default()); + let taken = core::mem::take(matched); let SecContextState { protocol_stage: SecContextStage::EdhocResponderProcessedM1 { @@ -960,6 +963,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler M::Code::new(coap_numbers::code::UNAUTHORIZED).map_err(|x| Own(x.into()))?, ); } - }) + }; + Ok(()) } } diff --git a/src/riot-rs-coap/src/lib.rs b/src/riot-rs-coap/src/lib.rs index 0328cc4d7..e31e05a78 100644 --- a/src/riot-rs-coap/src/lib.rs +++ b/src/riot-rs-coap/src/lib.rs @@ -12,12 +12,11 @@ // Moving work from https://github.com/embassy-rs/embassy/pull/2519 in here for the time being mod udp_nal; -use coap_handler_implementations::{HandlerBuilder, ReportingHandlerBuilder}; +use coap_handler_implementations::ReportingHandlerBuilder; use coapcore::seccontext; -use critical_section::Mutex; use embassy_net::udp::{PacketMetadata, UdpSocket}; use embassy_sync::once_lock::OnceLock; -use riot_rs_debug::log::*; +use riot_rs_debug::log::info; use riot_rs_embassy::sendcell::SendCell; use static_cell::StaticCell; @@ -35,6 +34,11 @@ static CLIENT: OnceLock< /// /// This can only be run once, as it sets up a system wide CoAP handler. pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Reporting) -> ! { + use hexlit::hex; + const R: &[u8] = &hex!("72cc4761dbd4c78f758931aa589d348d1ef874a7e303ede2f140dcf3e6aa4aac"); + + static COAP: StaticCell> = StaticCell::new(); + let stack = riot_rs_embassy::network::network_stack().await.unwrap(); // FIXME trim to CoAP requirements @@ -50,7 +54,6 @@ pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Report &mut tx_meta, &mut tx_buffer, ); - use embedded_nal_async::UnconnectedUdp; info!("Starting up CoAP server"); @@ -61,8 +64,6 @@ pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Report .await .unwrap(); - use hexlit::hex; - const R: &[u8] = &hex!("72cc4761dbd4c78f758931aa589d348d1ef874a7e303ede2f140dcf3e6aa4aac"); let own_identity = ( &lakers::CredentialRPK::new(lakers::EdhocMessageBuffer::new_from_slice(&hex!("A2026008A101A5010202410A2001215820BBC34960526EA4D32E940CAD2A234148DDC21791A12AFBCBAC93622046DD44F02258204519E257236B2A0CE2023F0931F1F386CA7AFDA64FCDE0108C224C51EABF6072")).expect("Credential should be small enough")).expect("Credential should be processable"), R, @@ -77,8 +78,7 @@ pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Report info!("Server is ready."); - static COAP: StaticCell> = StaticCell::new(); - let coap = COAP.init_with(|| embedded_nal_coap::CoAPShared::new()); + let coap = COAP.init_with(embedded_nal_coap::CoAPShared::new); let (client, server) = coap.split(); CLIENT .init(SendCell::new_async(client).await) diff --git a/src/riot-rs-coap/src/udp_nal/mod.rs b/src/riot-rs-coap/src/udp_nal/mod.rs index 6c9478314..fc2aebe7b 100644 --- a/src/riot-rs-coap/src/udp_nal/mod.rs +++ b/src/riot-rs-coap/src/udp_nal/mod.rs @@ -1,14 +1,14 @@ -//! UDP sockets usable through [embedded_nal_async] +//! UDP sockets usable through [`embedded_nal_async`] //! -//! The full [embedded_nal_async::UdpStack] is *not* implemented at the moment: As its API allows -//! arbitrary creation of movable sockets, embassy's [udp::UdpSocket] type could only be crated if +//! The full [`embedded_nal_async::UdpStack`] is *not* implemented at the moment: As its API allows +//! arbitrary creation of movable sockets, embassy's [`udp::UdpSocket`] type could only be crated if //! the NAL stack had a pre-allocated pool of sockets with their respective buffers. Nothing rules //! out such a type, but at the moment, only the bound or connected socket types are implemented -//! with their own constructors from an embassy [crate::Stack] -- for many applications, those are +//! with their own constructors from an embassy [`crate::Stack`] -- for many applications, those are //! useful enough. (FIXME: Given we construct from Socket, Stack could really be implemented on //! `Cell>` by `.take()`ing, couldn't it?) //! -//! The constructors of the various socket types mimic the UdpStack's socket creation functions, +//! The constructors of the various socket types mimic the [`UdpStack`]'s socket creation functions, //! but take an owned (uninitialized) Socket instead of a shared stack. //! //! No `bind_single` style constructor is currently provided. FIXME: Not sure we have all the @@ -25,6 +25,10 @@ mod util; pub use util::Error; use util::{is_unspec_ip, sockaddr_nal2smol, sockaddr_smol2nal}; +#[expect( + dead_code, + reason = "pub item is being prepared for embedded-nal-async where it will be reachable publicly" +)] pub struct ConnectedUdp<'a> { remote: IpEndpoint, // The local port is stored in the socket, as it gets bound. This value is populated lazily: @@ -38,13 +42,19 @@ pub struct ConnectedUdp<'a> { /// A UDP socket that has been bound locally (either to a unique address or just to a port) /// -/// Its operations are accessible through the [nal::UnconnectedUdp] trait. +/// Its operations are accessible through the [`nal::UnconnectedUdp`] trait. pub struct UnconnectedUdp<'a> { socket: udp::UdpSocket<'a>, } +#[allow( + dead_code, + clippy::unused_async, + clippy::missing_errors_doc, + reason = "pub item is being prepared for embedded-nal-async where it will be reachable publicly" +)] impl<'a> ConnectedUdp<'a> { - /// Create a ConnectedUdp by assigning it a remote and a concrete local address + /// Create a [`ConnectedUdp`] by assigning it a remote and a concrete local address /// /// ## Prerequisites /// @@ -67,8 +77,8 @@ impl<'a> ConnectedUdp<'a> { }) } - /// Create a ConnectedUdp by assigning it a remote and a local address (the latter may happen - /// lazily) + /// Create a [`ConnectedUdp`] by assigning it a remote and a local address (the latter may + /// happen lazily) /// /// ## Prerequisites /// @@ -80,8 +90,14 @@ impl<'a> ConnectedUdp<'a> { } } +#[allow( + dead_code, + clippy::unused_async, + clippy::missing_errors_doc, + reason = "pub item is being prepared for embedded-nal-async where it will be reachable publicly" +)] impl<'a> UnconnectedUdp<'a> { - /// Create an UnconnectedUdp. + /// Create an [`UnconnectedUdp`]. /// /// The `local` address may be anything from fully specified (address and port) to fully /// unspecified (port 0, all-zeros address). diff --git a/src/riot-rs-coap/src/udp_nal/util.rs b/src/riot-rs-coap/src/udp_nal/util.rs index 5d04060d8..fe6556258 100644 --- a/src/riot-rs-coap/src/udp_nal/util.rs +++ b/src/riot-rs-coap/src/udp_nal/util.rs @@ -1,8 +1,18 @@ -//! Helpers for udp_nal -- conversion and error types +//! Helpers for [`udp_nal`] -- conversion and error types use embassy_net::{udp, IpAddress, IpEndpoint}; use embedded_nal_async as nal; +/// Converts socket address types between [`embedded_nal_async`] and [`embassy_net`] (internally +/// `smol`). +/// +/// # Errors +/// +/// This produces an error if an address family is unavailable. +#[allow( + clippy::unnecessary_wraps, + reason = "errors are currently only impossible because for crate feature synchronization reasons, all cfg handling is commented out" +)] pub(super) fn sockaddr_nal2smol(sockaddr: nal::SocketAddr) -> Result { match sockaddr { #[allow(unused)] @@ -44,7 +54,7 @@ pub(super) fn sockaddr_smol2nal(endpoint: IpEndpoint) -> nal::SocketAddr { /// Is the IP address in this type the unspecified address? /// -/// FIXME: What of ::ffff:0.0.0.0? Is that expected to bind to all v4 addresses? +/// FIXME: What of `::ffff:0.0.0.0`? Is that expected to bind to all v4 addresses? pub(super) fn is_unspec_ip(addr: nal::SocketAddr) -> bool { match addr { nal::SocketAddr::V4(sockaddr) => sockaddr.ip().octets() == [0; 4], @@ -52,9 +62,13 @@ pub(super) fn is_unspec_ip(addr: nal::SocketAddr) -> bool { } } -/// Unified error type for [embedded_nal_async] operations on UDP sockets +/// Unified error type for [`embedded_nal_async`] operations on UDP sockets #[derive(Debug)] #[non_exhaustive] +#[allow( + clippy::enum_variant_names, + reason = "false positive -- they're not called SomethingError because they are a Self (which is named Error), but because they contain a type SomethingError" +)] pub enum Error { /// Error stemming from failure to send RecvError(udp::RecvError), @@ -64,22 +78,20 @@ pub enum Error { BindError(udp::BindError), /// Error stemming from failure to represent the given address family for lack of enabled /// embassy-net features + #[expect(dead_code, reason = "feature selection currently disabled")] AddressFamilyUnavailable, } impl embedded_io_async::Error for Error { fn kind(&self) -> embedded_io_async::ErrorKind { match self { - Self::SendError(udp::SendError::NoRoute) => { - embedded_io_async::ErrorKind::AddrNotAvailable - } - Self::BindError(udp::BindError::NoRoute) => { + Self::SendError(udp::SendError::NoRoute) | Self::BindError(udp::BindError::NoRoute) => { embedded_io_async::ErrorKind::AddrNotAvailable } Self::AddressFamilyUnavailable => embedded_io_async::ErrorKind::AddrNotAvailable, // These should not happen b/c our sockets are typestated. - Self::SendError(udp::SendError::SocketNotBound) => embedded_io_async::ErrorKind::Other, - Self::BindError(udp::BindError::InvalidState) => embedded_io_async::ErrorKind::Other, + Self::SendError(udp::SendError::SocketNotBound) | + Self::BindError(udp::BindError::InvalidState) | // This should not happen b/c in embedded_nal_async this is not expressed through an // error. // FIXME we're not there in this impl yet.