Treewide fixes for warnigns and refactorings, including a double free

Tests were added to address fixes that were made; most of the changes
are merely refactorings.

Merges: #62

src/auto_init.rs

@@ -1,6 +1,6 @@
 //! Tools for declaring a function that is run during initialization
 //!
-//! The [auto_init!] macro is this module's main product.
+//! The [`auto_init!`](super::auto_init!) macro is this module's main product.
 
 /// Wrapper around [riot_sys::auto_init_module_t]
 ///
@@ -13,7 +13,7 @@ impl AutoInitModule {
     /// Initializer for module auto-initialization
     ///
     /// Do not call this directly: Its result must be placed in a static in a special section in
-    /// memory, which is handled by the [`auto_init!`] macro.
+    /// memory, which is handled by the [`auto_init!`](super::auto_init!) macro.
     pub const fn new(
         init_function: extern "C" fn(),
         priority: u16,

src/bluetil.rs

@@ -31,7 +31,7 @@ pub enum Error {
 
 impl From<crate::error::NumericError> for Error {
     fn from(e: crate::error::NumericError) -> Error {
-        match e.number as _ {
+        match e.number() as _ {
             riot_sys::BLUETIL_AD_NOTFOUND => Error::NotFound,
             riot_sys::BLUETIL_AD_NOMEM => Error::NoMem,
             _ => panic!("Invalid bluetil error"),

src/gcoap.rs

@@ -216,7 +216,7 @@ unsafe extern "C" fn link_encoder<H: WithLinkEncoder>(
     let h: &H = unsafe { &*((*resource).context as *const _) };
 
     let buf = buf as *mut u8; // cast away signedness of char
-    let mut buf = if buf.is_null() {
+    let buf = if buf.is_null() {
         None
     } else {
         Some(core::slice::from_raw_parts_mut(buf, buf_len as _))
@@ -259,7 +259,7 @@ impl<'a, H> SingleHandlerListener<'a, H>
 where
     H: 'a + Handler + WithLinkEncoder,
 {
-    /// Like [`new()`], but utilizing that the handler is also [WithLinkEncoder] and can thus influence
+    /// Like [`Self::new()`], but utilizing that the handler is also [WithLinkEncoder] and can thus influence
     /// what is reported when the default .well-known/core handler is queried.
     pub fn new_with_link_encoder(
         path: &'a core::ffi::CStr,
@@ -344,7 +344,6 @@ pub trait WithLinkEncoder {
 }
 
 use riot_sys::{
-    coap_get_total_hdr_len,
     coap_opt_add_opaque,
     coap_opt_add_uint,
     coap_opt_get_next,
@@ -376,11 +375,6 @@ impl PacketBuffer {
         }) as u8 // odd return type in C
     }
 
-    /// Wrapper for coap_get_total_hdr_len
-    fn get_total_hdr_len(&self) -> usize {
-        (unsafe { coap_get_total_hdr_len(crate::inline_cast(self.pkt)) }) as usize
-    }
-
     /// Wrapper for gcoap_resp_init
     ///
     /// As it is used and wrapped here, this makes GCOAP_RESP_OPTIONS_BUF bytes unusable, but

src/gnrc.rs

@@ -5,7 +5,8 @@ pub mod ipv6;
 
 pub mod netapi;
 pub mod netreg;
-pub mod pktbuf;
+#[deprecated(note = "moved to gnrc_pktbuf toplevel module")]
+pub use crate::gnrc_pktbuf as pktbuf;
 
 use riot_sys::{gnrc_netif_iter, gnrc_netif_t};
 

src/gnrc/netreg.rs

@@ -1,7 +1,7 @@
+#[cfg(feature = "with_msg_v2")]
 use core::mem::MaybeUninit;
 
-use riot_sys::{gnrc_netreg_entry_t, gnrc_netreg_register, gnrc_netreg_unregister, gnrc_nettype_t};
-
+#[cfg(feature = "with_msg_v2")]
 use crate::error::NegativeErrorExt;
 
 // Transmuting the pointer into a Pktsnip does the right thing by treating it as a smart

src/gnrc/pktbuf.rs → src/gnrc_pktbuf.rs

@@ -11,7 +11,6 @@ use riot_sys::{
     gnrc_pktbuf_realloc_data,
     gnrc_pktbuf_release_error,
     gnrc_pktsnip_t,
-    gnrc_udp_hdr_build,
     GNRC_NETERR_SUCCESS,
 };
 
@@ -138,6 +137,8 @@ impl<M: Mode> Pktsnip<M> {
         src: core::num::NonZeroU16,
         dst: core::num::NonZeroU16,
     ) -> Result<Pktsnip<Writable>, NotEnoughSpace> {
+        use riot_sys::gnrc_udp_hdr_build;
+
         let snip = unsafe { gnrc_udp_hdr_build(self.ptr, src.into(), dst.into()) };
         if snip == 0 as *mut _ {
             // All other errors are caught by the signature
@@ -253,13 +254,19 @@ impl<'a> Pktsnip<Writable> {
         size: usize,
         nettype: gnrc_nettype_t,
     ) -> Result<Self, NotEnoughSpace> {
-        let next = next.map(|s| s.ptr).unwrap_or(0 as *mut _);
-        let snip =
-            unsafe { gnrc_pktbuf_add(next, data as *const _, size.try_into().unwrap(), nettype) };
+        let next_ptr = next.as_ref().map(|s| s.ptr).unwrap_or(0 as *mut _);
+        forget(next);
+        let snip = unsafe {
+            gnrc_pktbuf_add(
+                next_ptr,
+                data as *const _,
+                size.try_into().unwrap(),
+                nettype,
+            )
+        };
         if snip == 0 as *mut _ {
             return Err(NotEnoughSpace);
         }
-        forget(next);
         Ok(unsafe { Pktsnip::<Writable>::from_ptr(snip) })
     }
 
@@ -284,7 +291,13 @@ impl<'a> Pktsnip<Writable> {
 
 impl<M: Mode> ::core::fmt::Debug for Pktsnip<M> {
     fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
+        let mode = core::any::type_name::<M>();
+        let mode = mode
+            .rsplit("::")
+            .next()
+            .expect("Type name contains :: because it is part of a module");
         f.debug_struct("Pktsnip")
+            .field("M", &mode)
             .field("length", &self.len())
             .field("snips", &self.count())
             .finish()

src/interrupt.rs

@@ -5,7 +5,7 @@
 //! * Utility functions can disable interrupts (creating critical sections), check whether
 //!   interrupts are enabled or to determine whether code is executed in a thread or an ISR.
 //!
-//! * Some functions (eg. [`ZTimer::set_ticks_during`](crate::ztimer::ZTimer::set_ticks_during))
+//! * Some functions (eg. [`ZTimer::set_ticks_during`](crate::ztimer::Clock::set_during))
 //!   take callbacks that will be called in an interrupt context.
 //!
 //!   These are typechecked to be Send, as they are moved from the thread to the interrupt context.

src/lib.rs

@@ -112,6 +112,9 @@ pub mod thread;
 pub mod gcoap;
 #[cfg(riot_module_gnrc)]
 pub mod gnrc;
+// Note that this can also exist without gnrc
+#[cfg(riot_module_gnrc_pktbuf)]
+pub mod gnrc_pktbuf;
 #[cfg(riot_module_gnrc)]
 pub mod gnrc_util;
 #[cfg(riot_module_periph_i2c)]

src/main_module.rs

@@ -1,6 +1,6 @@
 //! Tools for providing a RIOT main function
 //!
-//! The main contribution of this module is the [riot_main] macro.
+//! The main contribution of this module is the [`riot_main!`](super::riot_main!) macro.
 //!
 //! The alternative to using that (other than doing it manually) is to have C code along with the
 //! Rust application that occupies the main function.
@@ -10,6 +10,7 @@
 //! C code.
 
 use crate::stdio::println;
+use crate::thread::{EndToken, StartToken};
 
 // General alternative to this module: Build the extern "C" main all the time and request that the
 // application implement a named function. I never got the main function to be carried to the
@@ -55,9 +56,9 @@ impl<F: Fn() -> T, T: Termination> UsableAsMain<[u8; 1]> for F {
     }
 }
 
-impl<F: Fn(crate::thread::StartToken) -> crate::never::Never> Sealed<[u8; 2]> for F {}
+impl<F: Fn(StartToken) -> crate::never::Never> Sealed<[u8; 2]> for F {}
 
-impl<F: Fn(crate::thread::StartToken) -> crate::never::Never> UsableAsMain<[u8; 2]> for F {
+impl<F: Fn(StartToken) -> crate::never::Never> UsableAsMain<[u8; 2]> for F {
     unsafe fn call_main(&self) -> i32 {
         // unsafe: By construction of the C main function this only happens at startup time
         // with a thread that hasn't done anything relevant before.
@@ -67,11 +68,9 @@ impl<F: Fn(crate::thread::StartToken) -> crate::never::Never> UsableAsMain<[u8;
     }
 }
 
-impl<F: Fn(crate::thread::StartToken) -> ((), crate::thread::EndToken)> Sealed<[u8; 3]> for F {}
+impl<F: Fn(StartToken) -> ((), EndToken)> Sealed<[u8; 3]> for F {}
 
-impl<F: Fn(crate::thread::StartToken) -> ((), crate::thread::EndToken)> UsableAsMain<[u8; 3]>
-    for F
-{
+impl<F: Fn(StartToken) -> ((), EndToken)> UsableAsMain<[u8; 3]> for F {
     unsafe fn call_main(&self) -> i32 {
         // unsafe: By construction of the C main function this only happens at startup time
         // with a thread that hasn't done anything relevant before.

src/msg.rs

@@ -28,7 +28,7 @@ pub enum MsgSender {
 
 impl MsgSender {
     fn from_pid(pid: kernel_pid_t) -> Self {
-        if pid == crate::thread::pid_converted::KERNEL_PID_ISR {
+        if pid == crate::thread::KERNEL_PID_ISR {
             MsgSender::ISR
         } else {
             KernelPID::new(pid)

src/never.rs

@@ -3,6 +3,7 @@
 ///
 /// From <https://github.com/rust-lang/rust/issues/43301#issuecomment-912390203>, adjusted for
 /// usability with pub interfaces by using a pub trait in a private module (sealing).
+#[cfg(not(feature = "actual_never_type"))]
 use crate::helpers::*;
 
 #[cfg(not(feature = "actual_never_type"))]

src/panic.rs

@@ -24,46 +24,45 @@ fn panic(info: &::core::panic::PanicInfo) -> ! {
                 cstr::cstr!("RUST PANIC").as_ptr() as _,
             )
         };
-        unreachable!()
-    }
-
-    // I *guess* it's OK for a panic to simply make a thread into a zombie -- this does allow other
-    // threads (including spawned Rust threads) to continue, but my layman's understanding of
-    // panicking is that that's OK because whatever we were just mutating can simply never be used
-    // by someone else ever again.
+    } else {
+        // I *guess* it's OK for a panic to simply make a thread into a zombie -- this does allow other
+        // threads (including spawned Rust threads) to continue, but my layman's understanding of
+        // panicking is that that's OK because whatever we were just mutating can simply never be used
+        // by someone else ever again.
 
-    let me = thread::get_pid();
+        let me = thread::get_pid();
 
-    if cfg!(feature = "panic_handler_format") {
-        use crate::stdio::println;
+        if cfg!(feature = "panic_handler_format") {
+            use crate::stdio::println;
 
-        println!(
-            "Error in thread {:?} ({}):",
-            me,
-            me.get_name().unwrap_or("unnamed")
-        );
-        println!("{}", info);
-    } else {
-        let mut stdio = crate::stdio::Stdio {};
-        use core::fmt::Write;
-        let _ = stdio.write_str("Panic in thread ");
-        let _ = stdio.write_str(me.get_name().unwrap_or("unnamed"));
-        let _ = stdio.write_str("!\n");
-    }
+            println!(
+                "Error in thread {:?} ({}):",
+                me,
+                me.get_name().unwrap_or("unnamed")
+            );
+            println!("{}", info);
+        } else {
+            let mut stdio = crate::stdio::Stdio {};
+            use core::fmt::Write;
+            let _ = stdio.write_str("Panic in thread ");
+            let _ = stdio.write_str(me.get_name().unwrap_or("unnamed"));
+            let _ = stdio.write_str("!\n");
+        }
 
-    if cfg!(feature = "panic_handler_crash") {
-        unsafe {
-            riot_sys::core_panic(
-                riot_sys::core_panic_t_PANIC_GENERAL_ERROR,
-                cstr::cstr!("RUST PANIC").as_ptr() as _,
-            )
+        if cfg!(feature = "panic_handler_crash") {
+            unsafe {
+                riot_sys::core_panic(
+                    riot_sys::core_panic_t_PANIC_GENERAL_ERROR,
+                    cstr::cstr!("RUST PANIC").as_ptr() as _,
+                )
+            }
         }
-    }
 
-    // Not trying any unwinding -- this thread is just dead, won't be re-claimed, any mutexes it
-    // holds are just held indefinitely rather than throwing poison errors.
-    loop {
-        thread::sleep();
+        // Not trying any unwinding -- this thread is just dead, won't be re-claimed, any mutexes it
+        // holds are just held indefinitely rather than throwing poison errors.
+        loop {
+            thread::sleep();
+        }
     }
 }
 

src/saul.rs

@@ -17,12 +17,8 @@
 //!
 //! [SAUL]: https://doc.riot-os.org/group__drivers__saul.html
 
-use riot_sys as raw;
-use riot_sys::libc;
-
 use crate::error;
 use crate::helpers::PointerToCStr;
-use crate::Never;
 use error::NegativeErrorExt;
 
 pub mod registration;
@@ -95,10 +91,9 @@ impl RegistryEntry {
             unsafe { riot_sys::saul_reg_write(self.0, &value.values as *const _ as *mut _) }
                 .negative_to_error()?;
         if length != value.length.into() {
-            // FIXME is this the best way to express the error?
-            Err(error::NumericError {
-                number: length as isize,
-            })
+            // It's not pretty to synthesize an error here, but neither would be expressing the
+            // partial write in the context of lengthful phydat items.
+            Err(error::NumericError::from_constant(riot_sys::EINVAL as _))
         } else {
             Ok(())
         }
@@ -404,8 +399,10 @@ impl Unit {
     // we can just switch over before they go.
     #[deprecated(note = "Use the GForce variant instead")]
     pub const G: Self = Unit::GForce;
+    #[allow(non_upper_case_globals)]
     #[deprecated(note = "Use the Gram variant instead")]
     pub const Gr: Self = Unit::Gram;
+    #[allow(non_upper_case_globals)]
     #[deprecated(note = "Use the Gauss variant instead")]
     pub const Gs: Self = Unit::Gauss;
 

src/saul/registration.rs

@@ -33,8 +33,9 @@ pub trait Drivable: Sized {
     /// Set to true if `read` is implemented.
     ///
     /// Doing this on the type level (rather than having read and write return a more
-    /// differentiated error) allows the driver to point to the shared [riot_sys::saul_notsup]
-    /// handler rather than to monomorphize a custom erring handler for each device.
+    /// differentiated error) allows the driver to point to the shared [riot_sys::saul_read_notsup]
+    /// / [riot_sys::saul_write_notsup] handler rather than to monomorphize a custom erring handler
+    /// for each device.
     const HAS_READ: bool = false;
     /// Set to true if `write` is implemented.
     const HAS_WRITE: bool = false;
@@ -50,7 +51,7 @@ pub trait Drivable: Sized {
     /// Set the state of an actuator, or reconfigure a sensor
     ///
     /// A &self is passed in on write because there could be concurrent access from multiple SAUL
-    /// users. One option of handling this is to implement Drivable for Mutex<T>.
+    /// users. One option of handling this is to implement Drivable for `Mutex<T>`.
     ///
     /// Note that due to the way SAUL is structured, the drivable can not know the number of
     /// entries which the user intended to set. The Drivable trait always builds the Rust Phydat

src/shell.rs

@@ -2,8 +2,8 @@
 //!
 //! This module can be used in two ways:
 //!
-//! * Declare static commands using [static_command]; these only take a `fn` (not a closure)
-//!   because shell commands don't have an arg pointer.
+//! * Declare static commands using [`static_command!`](crate::static_command!); these only take a
+//!   `fn` (not a closure) because shell commands don't have an arg pointer.
 //!
 //!   This works even in RIOT modules that are included in a C application that starts a shell, and
 //!   show up in shells created through Rust without explicit inclusion.
@@ -20,8 +20,8 @@
 //! argument. This does allow the Rust wrappers to "just so" use a closure as a command handler,
 //! but also needs a lot of code.
 //!
-//! That complexity is not pulled in when only using [static_command] and running on an otherwise
-//! empty command list.
+//! That complexity is not pulled in when only using [`static_command!`](crate::static_command!)
+//! and running on an otherwise empty command list.
 
 use crate::{mutex, stdio};
 use core::ffi::CStr;
@@ -108,11 +108,13 @@ pub unsafe trait CommandListInternals: Sized {
         let sleeve = lock
             .as_ref()
             .expect("Callback called while no shell set up as running");
-        // unsafe: A suitable callback is always configured. We can make a &mut out of it for as
-        // long as we hold the lock.
-        let root = unsafe { &mut *(sleeve.0 as *mut Self) };
-        let result = root.find_self_and_run(argc, argv, command_index);
-        drop(root);
+        let result;
+        {
+            // unsafe: A suitable callback is always configured. We can make a &mut out of it for as
+            // long as we hold the lock.
+            let root = unsafe { &mut *(sleeve.0 as *mut Self) };
+            result = root.find_self_and_run(argc, argv, command_index);
+        }
         drop(lock);
         result
     }