Minor MGS tweaks to support hubris work (#1396)

* IgnitionCommand: impl PartialEq
* Replace `SpServer` with `handle_message()` (caller-managed output buffer)

gateway-messages/src/lib.rs

@@ -294,7 +294,9 @@ mod bulk_ignition_state_serde {
     }
 }
 
-#[derive(Debug, Clone, Copy, SerializedSize, Serialize, Deserialize)]
+#[derive(
+    Debug, Clone, Copy, SerializedSize, Serialize, Deserialize, PartialEq,
+)]
 pub enum IgnitionCommand {
     PowerOn,
     PowerOff,

gateway-messages/src/sp_impl.rs

@@ -169,86 +169,74 @@ impl Iterator for SerialConsolePackets<'_, '_> {
     }
 }
 
-#[derive(Debug)]
-pub struct SpServer {
-    buf: [u8; SpMessage::MAX_SIZE],
-}
+/// Handle a single incoming message.
+///
+/// The incoming message is described by `sender` (the remote address of the
+/// sender), `port` (the local port the message arived on), and `data` (the raw
+/// message). It will be deserialized, and the appropriate method will be called
+/// on `handler` to craft a response. The response will then be serialized into
+/// `out`, and returned `Ok(n)` value specifies length of the serialized
+/// response.
+pub fn handle_message<H: SpHandler>(
+    sender: SocketAddrV6,
+    port: SpPort,
+    data: &[u8],
+    handler: &mut H,
+    out: &mut [u8; SpMessage::MAX_SIZE],
+) -> Result<usize, Error> {
+    // parse request, with sanity checks on sizes
+    if data.len() > Request::MAX_SIZE {
+        return Err(Error::DataTooLarge);
+    }
+    let (request, leftover) = hubpack::deserialize::<Request>(data)?;
+    if !leftover.is_empty() {
+        return Err(Error::LeftoverData);
+    }
 
-impl Default for SpServer {
-    fn default() -> Self {
-        Self { buf: [0; SpMessage::MAX_SIZE] }
+    // `version` is intentionally the first 4 bytes of the packet; we could
+    // check it before trying to deserialize?
+    if request.version != version::V1 {
+        return Err(Error::UnsupportedVersion(request.version));
     }
-}
 
-impl SpServer {
-    /// Handler for incoming UDP requests.
-    ///
-    /// `data` should be a UDP packet that has arrived from `sender` on `port`.
-    /// It will be parsed (into a [`Request`]), the appropriate method will be
-    /// called on `handler`, and a serialized response will be returned, which
-    /// the caller should send back to the requester.
-    pub fn dispatch<H: SpHandler>(
-        &mut self,
-        sender: SocketAddrV6,
-        port: SpPort,
-        data: &[u8],
-        handler: &mut H,
-    ) -> Result<&[u8], Error> {
-        // parse request, with sanity checks on sizes
-        if data.len() > Request::MAX_SIZE {
-            return Err(Error::DataTooLarge);
+    // call out to handler to provide response
+    let result = match request.kind {
+        RequestKind::Discover => {
+            handler.discover(sender, port).map(ResponseKind::Discover)
         }
-        let (request, leftover) = hubpack::deserialize::<Request>(data)?;
-        if !leftover.is_empty() {
-            return Err(Error::LeftoverData);
+        RequestKind::IgnitionState { target } => handler
+            .ignition_state(sender, port, target)
+            .map(ResponseKind::IgnitionState),
+        RequestKind::BulkIgnitionState => handler
+            .bulk_ignition_state(sender, port)
+            .map(ResponseKind::BulkIgnitionState),
+        RequestKind::IgnitionCommand { target, command } => handler
+            .ignition_command(sender, port, target, command)
+            .map(|()| ResponseKind::IgnitionCommandAck),
+        RequestKind::SpState => {
+            handler.sp_state(sender, port).map(ResponseKind::SpState)
         }
-
-        // `version` is intentionally the first 4 bytes of the packet; we could
-        // check it before trying to deserialize?
-        if request.version != version::V1 {
-            return Err(Error::UnsupportedVersion(request.version));
-        }
-
-        // call out to handler to provide response
-        let result = match request.kind {
-            RequestKind::Discover => {
-                handler.discover(sender, port).map(ResponseKind::Discover)
-            }
-            RequestKind::IgnitionState { target } => handler
-                .ignition_state(sender, port, target)
-                .map(ResponseKind::IgnitionState),
-            RequestKind::BulkIgnitionState => handler
-                .bulk_ignition_state(sender, port)
-                .map(ResponseKind::BulkIgnitionState),
-            RequestKind::IgnitionCommand { target, command } => handler
-                .ignition_command(sender, port, target, command)
-                .map(|()| ResponseKind::IgnitionCommandAck),
-            RequestKind::SpState => {
-                handler.sp_state(sender, port).map(ResponseKind::SpState)
-            }
-            RequestKind::SerialConsoleWrite(packet) => handler
-                .serial_console_write(sender, port, packet)
-                .map(|()| ResponseKind::SerialConsoleWriteAck),
-        };
-
-        // we control `SpMessage` and know all cases can successfully serialize
-        // into `self.buf`
-        let response = SpMessage {
-            version: version::V1,
-            kind: SpMessageKind::Response {
-                request_id: request.request_id,
-                result,
-            },
-        };
-        let n = match hubpack::serialize(&mut self.buf, &response) {
-            Ok(n) => n,
-            Err(_) => panic!(),
-        };
-
-        // Do we want some mechanism for remembering `n` if our caller wants to
-        // resend this packet, which would have to happen before calling this
-        // method again? For now (and maybe forever), force them to just call us
-        // again, and we'll reserialize.
-        Ok(&self.buf[..n])
-    }
+        RequestKind::SerialConsoleWrite(packet) => handler
+            .serial_console_write(sender, port, packet)
+            .map(|()| ResponseKind::SerialConsoleWriteAck),
+    };
+
+    // we control `SpMessage` and know all cases can successfully serialize
+    // into `self.buf`
+    let response = SpMessage {
+        version: version::V1,
+        kind: SpMessageKind::Response {
+            request_id: request.request_id,
+            result,
+        },
+    };
+
+    // We know `response` is well-formed and fits into `out` (since it's
+    // statically sized for `SpMessage`), so we can unwrap serialization.
+    let n = match hubpack::serialize(&mut out[..], &response) {
+        Ok(n) => n,
+        Err(_) => panic!(),
+    };
+
+    Ok(n)
 }

gateway-sp-comms/src/recv_handler/mod.rs

@@ -275,7 +275,11 @@ impl RecvHandler {
                 msg
             }
             Err(err) => {
-                error!(&self.log, "discarding malformed message ({})", err);
+                error!(
+                    &self.log, "discarding malformed message";
+                    "err" => %err,
+                    "raw" => ?buf,
+                );
                 return;
             }
         };

sp-sim/src/gimlet.rs

@@ -10,7 +10,7 @@ use crate::{Responsiveness, SimulatedSp};
 use anyhow::{anyhow, bail, Context, Result};
 use async_trait::async_trait;
 use futures::future;
-use gateway_messages::sp_impl::{SerialConsolePacketizer, SpHandler, SpServer};
+use gateway_messages::sp_impl::{SerialConsolePacketizer, SpHandler};
 use gateway_messages::version;
 use gateway_messages::DiscoverResponse;
 use gateway_messages::ResponseError;
@@ -416,15 +416,15 @@ impl UdpTask {
     }
 
     async fn run(mut self) -> Result<()> {
-        let mut server = SpServer::default();
+        let mut out_buf = [0; SpMessage::MAX_SIZE];
         let mut responsiveness = Responsiveness::Responsive;
         loop {
             select! {
                 recv0 = self.udp0.recv_from() => {
                     if let Some((resp, addr)) = server::handle_request(
                         &mut self.handler,
                         recv0,
-                        &mut server,
+                        &mut out_buf,
                         responsiveness,
                         SpPort::One,
                     ).await? {
@@ -436,7 +436,7 @@ impl UdpTask {
                     if let Some((resp, addr)) = server::handle_request(
                         &mut self.handler,
                         recv1,
-                        &mut server,
+                        &mut out_buf,
                         responsiveness,
                         SpPort::Two,
                     ).await? {

sp-sim/src/server.rs

@@ -8,10 +8,11 @@ use anyhow::anyhow;
 use anyhow::bail;
 use anyhow::Context;
 use anyhow::Result;
+use gateway_messages::sp_impl;
 use gateway_messages::sp_impl::SpHandler;
-use gateway_messages::sp_impl::SpServer;
 use gateway_messages::Request;
 use gateway_messages::SerializedSize;
+use gateway_messages::SpMessage;
 use gateway_messages::SpPort;
 use slog::debug;
 use slog::error;
@@ -122,7 +123,7 @@ pub fn logger(config: &Config) -> Result<Logger> {
 pub(crate) async fn handle_request<'a, H: SpHandler>(
     handler: &mut H,
     recv: Result<(&[u8], SocketAddrV6)>,
-    server: &'a mut SpServer,
+    out: &'a mut [u8; SpMessage::MAX_SIZE],
     responsiveness: Responsiveness,
     port_num: SpPort,
 ) -> Result<Option<(&'a [u8], SocketAddrV6)>> {
@@ -137,9 +138,8 @@ pub(crate) async fn handle_request<'a, H: SpHandler>(
     let (data, addr) =
         recv.with_context(|| format!("recv on {:?}", port_num))?;
 
-    let resp = server
-        .dispatch(addr, port_num, data, handler)
+    let n = sp_impl::handle_message(addr, port_num, data, handler, out)
         .map_err(|err| anyhow!("dispatching message failed: {:?}", err))?;
 
-    Ok(Some((resp, addr)))
+    Ok(Some((&out[..n], addr)))
 }

sp-sim/src/sidecar.rs

@@ -14,14 +14,15 @@ use anyhow::Result;
 use async_trait::async_trait;
 use futures::future;
 use gateway_messages::sp_impl::SpHandler;
-use gateway_messages::sp_impl::SpServer;
 use gateway_messages::BulkIgnitionState;
 use gateway_messages::DiscoverResponse;
 use gateway_messages::IgnitionCommand;
 use gateway_messages::IgnitionFlags;
 use gateway_messages::IgnitionState;
 use gateway_messages::ResponseError;
 use gateway_messages::SerialNumber;
+use gateway_messages::SerializedSize;
+use gateway_messages::SpMessage;
 use gateway_messages::SpPort;
 use gateway_messages::SpState;
 use slog::debug;
@@ -222,15 +223,15 @@ impl Inner {
     }
 
     async fn run(mut self) -> Result<()> {
-        let mut server = SpServer::default();
+        let mut out_buf = [0; SpMessage::MAX_SIZE];
         let mut responsiveness = Responsiveness::Responsive;
         loop {
             select! {
                 recv0 = self.udp0.recv_from() => {
                     if let Some((resp, addr)) = server::handle_request(
                         &mut self.handler,
                         recv0,
-                        &mut server,
+                        &mut out_buf,
                         responsiveness,
                         SpPort::One,
                     ).await? {
@@ -242,7 +243,7 @@ impl Inner {
                     if let Some((resp, addr)) = server::handle_request(
                         &mut self.handler,
                         recv1,
-                        &mut server,
+                        &mut out_buf,
                         responsiveness,
                         SpPort::Two,
                     ).await? {