Update to minicbor v2, remove error nonsense

Author: hawkw

Cargo.lock

@@ -92,7 +92,7 @@ leb128 = { version = "0.2.5", default-features = false }
 lpc55-pac = { version = "0.4", default-features = false }
 memchr = { version = "2.4", default-features = false }
 memoffset = { version = "0.6.5", default-features = false }
-minicbor = { version = "0.26.4", default-features = false }
+minicbor = { version = "2.0.0", default-features = false }
 multimap = { version = "0.8.3", default-features = false }
 nb = { version = "1", default-features = false }
 num = { version = "0.4", default-features = false }

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "minicbor-lease"
 version = "0.1.0"
-edition = "2021"
+edition = "2024"
 
 [dependencies]
 minicbor = { workspace = true }

lib/minicbor-lease/Cargo.toml

@@ -18,38 +18,43 @@ where
     ran_out_of_space: bool,
 }
 
-/// Errors returned by [`LeasedWriter::check_err`].
-#[derive(Copy, Clone, PartialEq)]
+/// Errors returned by the [`minicbor::encode::write::Write`] implementation for
+/// [`LeasedWriter`].
+#[derive(Copy, Clone, PartialEq, Debug)]
 pub enum Error {
     /// The other side of the lease has gone away.
     WentAway,
     /// Data could not be written as there was no room left in the lease.
     EndOfLease,
 }
 
-/// Errors returned by the [`minicbor::encode::write::Write`] implementation for
-/// [`LeasedWriter`].
-#[derive(Copy, Clone, PartialEq)]
-pub struct WriteError;
+impl core::fmt::Display for Error {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        f.write_str(match self {
+            Self::WentAway => "lease went away",
+            Self::EndOfLease => "end of lease",
+        })
+    }
+}
 
 impl<A> minicbor::encode::write::Write for LeasedWriter<'_, A>
 where
     A: idol_runtime::AttributeWrite,
 {
-    type Error = WriteError;
+    type Error = Error;
 
     fn write_all(&mut self, buf: &[u8]) -> Result<(), Self::Error> {
         let Some(end) = self.pos.checked_add(buf.len()) else {
             self.ran_out_of_space = true;
-            return Err(WriteError);
+            return Err(Error::EndOfLease);
         };
         if end >= self.lease.len() {
             self.ran_out_of_space = true;
-            return Err(WriteError);
+            return Err(Error::EndOfLease);
         }
         self.lease
             .write_range(self.pos..end, buf)
-            .map_err(|_| WriteError)?;
+            .map_err(|_| Error::WentAway)?;
 
         self.pos += buf.len();
 
@@ -101,49 +106,10 @@ where
         self.lease
     }
 
-    /// Returns `true` if the last `write_all` call to return an error failed
-    /// due to running out of space.
-    ///
-    /// This is an unfortunate workaround for a limitation of the `minicbor`
-    /// API: the errors our `Write` implementation can return are wrapped in an
-    /// `encode::Error`, and we can't actually get our errors *out* of that
-    /// wrapper, so there's no way to tell whether the error was because we ran
-    /// out of space in the buffer, or because the lease client went away. So,
-    /// we track it here, so that the caller can just ask us if we didn't have
-    /// space to encode something.
-    ///
-    /// Note that this is separate from `pos == lease.len()`, because we don't
-    /// actually write anything (and thus don't advance `pos`) if asked to write
-    /// a chunk of bytes that don't fit.
+    /// Returns `true` if the last `w
     pub fn ran_out_of_space(&self) -> bool {
         self.ran_out_of_space
     }
-
-    /// Determine whether an error returned by the [`minicbor::encode::Write`]
-    /// implementation indicates that there was no space left in the lease, or
-    /// that the client went away.
-    ///
-    /// This is an unfortunate workaround for a limitation of the `minicbor`
-    /// API: the errors our `Write` implementation can return are wrapped in an
-    /// `encode::Error`, and we can't actually get our errors *out* of that
-    /// wrapper, so there's no way to tell whether the error was becasue we ran
-    /// out of space in the buffer, or because the lease client went away. So,
-    /// we track it here, so that the caller can just ask us if we didn't have
-    /// space to encode something.
-    ///
-    pub fn check_err(&self, err: minicbor::encode::Error<WriteError>) -> Error {
-        if err.is_write() {
-            if self.ran_out_of_space {
-                Error::EndOfLease
-            } else {
-                Error::WentAway
-            }
-        } else {
-            // This is an encoder error, which we have no good way to
-            // recover from.
-            panic!()
-        }
-    }
 }
 
 impl From<Error> for idol_runtime::ClientError {

lib/minicbor-lease/src/lib.rs

@@ -20,7 +20,7 @@ use minicbor::CborLen;
 use minicbor_lease::LeasedWriter;
 use ringbuf::{counted_ringbuf, ringbuf_entry};
 use task_packrat_api::{EreportReadError, VpdIdentity};
-use userlib::{kipc, sys_get_timer, RecvMessage, TaskId};
+use userlib::{kipc, sys_get_timer, RecvMessage, TaskId, UnwrapLite};
 use zerocopy::IntoBytes;
 
 pub(crate) struct EreportStore {
@@ -160,11 +160,12 @@ impl EreportStore {
             &mut data,
         ));
 
-        fn check_err(
-            encoder: &minicbor::Encoder<LeasedWriter<'_, idol_runtime::W>>,
-            err: minicbor::encode::Error<minicbor_lease::WriteError>,
+        fn handle_encode_err(
+            err: minicbor::encode::Error<minicbor_lease::Error>,
         ) -> RequestError<EreportReadError> {
-            match encoder.writer().check_err(err) {
+            // These should always be write errors; everything we write should
+            // always encode successfully.
+            match err.into_write().unwrap_lite() {
                 minicbor_lease::Error::WentAway => ClientError::WentAway.fail(),
                 minicbor_lease::Error::EndOfLease => {
                     ClientError::BadLease.fail()
@@ -176,20 +177,7 @@ impl EreportStore {
         //
         // MGS expects us to always include this, and to just have it be
         // empty if we didn't send any metadata.
-        encoder
-            .begin_map()
-            // This pattern (which will occur every time we handle an encoder
-            // error in this function) is goofy, but is necessary to placate the
-            // borrow checker: the function passed to `map_err` must borrow the
-            // encoder so that it can check whether the error indicates that we
-            // ran out of space in the lease, or if the client disappeared.
-            // However, because `minicbor::Encoder`'s methods return a mutable
-            // borrow of the encoder, we must drop it first before borrowing it
-            // into the `map_err` closure. Thus, every `map_err` must be
-            // preceded by a `map` with a toilet closure that drops the mutable
-            // borrow. Yuck.
-            .map(|_| ())
-            .map_err(|e| check_err(&encoder, e))?;
+        encoder.begin_map().map_err(handle_encode_err)?;
 
         // If the requested restart ID matches the current restart ID, then read
         // from the requested ENA. If not, start at ENA 0.
@@ -218,8 +206,7 @@ impl EreportStore {
             if let Some(vpd) = vpd {
                 // Encode the metadata map.
                 self.encode_metadata(&mut encoder, vpd)
-                    .map(|_| ())
-                    .map_err(|e| check_err(&encoder, e))?;
+                    .map_err(handle_encode_err)?;
                 ringbuf_entry!(Trace::MetadataEncoded {
                     len: encoder
                         .writer()
@@ -233,10 +220,7 @@ impl EreportStore {
         };
 
         // End metadata map.
-        encoder
-            .end()
-            .map(|_| ())
-            .map_err(|e| check_err(&encoder, e))?;
+        encoder.end().map_err(handle_encode_err)?;
 
         let mut reports = 0;
         // Beginning with the first
@@ -290,24 +274,15 @@ impl EreportStore {
             if first_written_ena.is_none() {
                 first_written_ena = Some(r.ena);
                 // Start the ereport array
-                encoder
-                    .begin_array()
-                    .map(|_| ())
-                    .map_err(|e| check_err(&encoder, e))?;
+                encoder.begin_array().map_err(handle_encode_err)?;
             }
-            encoder
-                .encode(&entry)
-                .map(|_| ())
-                .map_err(|e| check_err(&encoder, e))?;
+            encoder.encode(&entry).map_err(handle_encode_err)?;
             reports += 1;
         }
 
         if let Some(start_ena) = first_written_ena {
             // End CBOR array, if we wrote anything.
-            encoder
-                .end()
-                .map(|_| ())
-                .map_err(|e| check_err(&encoder, e))?;
+            encoder.end().map_err(handle_encode_err)?;
 
             ringbuf_entry!(Trace::Reported {
                 start_ena,
@@ -335,7 +310,7 @@ impl EreportStore {
         &self,
         encoder: &mut minicbor::Encoder<LeasedWriter<'_, idol_runtime::W>>,
         vpd: &VpdIdentity,
-    ) -> Result<(), minicbor::encode::Error<minicbor_lease::WriteError>> {
+    ) -> Result<(), minicbor::encode::Error<minicbor_lease::Error>> {
         encoder
             .str("hubris_archive_id")?
             .bytes(&self.image_id[..])?;