review feedback from @jgallagher

Author: hawkw

faux-mgs/src/main.rs

@@ -93,10 +93,12 @@ struct Args {
     #[clap(long)]
     listen_port: Option<u16>,
 
-    /// Ereport port to bind to locally [default: 0 for client commands, 22223
-    /// for server commands]
-    #[clap(long)]
-    ereport_port: Option<u16>,
+    /// Ereport port to bind to locally
+    // Note that, unlike `listen_port`, this always defaults to 0, because we
+    // don't need to act as a server with a known port for the ereport
+    // protocol.
+    #[clap(long, default_value_t = 0)]
+    ereport_port: u16,
 
     /// Address to use to discover the SP. May be a specific SP's address to
     /// bypass multicast discovery.
@@ -549,15 +551,6 @@ impl Command {
             _ => 0,
         }
     }
-
-    fn default_ereport_port(&self) -> u16 {
-        match self {
-            // Server commands; use standard MGS port
-            Command::ServeHostPhase2 { .. } => ereport::MGS_PORT,
-            // Client commands: use port 0
-            _ => 0,
-        }
-    }
 }
 
 fn parse_tlvc_key(key: &str) -> Result<[u8; 4]> {
@@ -708,8 +701,6 @@ async fn main() -> Result<()> {
 
     let listen_port =
         args.listen_port.unwrap_or_else(|| args.command.default_listen_port());
-    let ereport_port =
-        args.listen_port.unwrap_or_else(|| args.command.default_ereport_port());
 
     // For faux-mgs, we'll serve all images present in the directory the user
     // requests, so don't cap the LRU cache size.
@@ -725,7 +716,7 @@ async fn main() -> Result<()> {
     .context("SharedSocket:bind() failed")?;
     let ereport_socket = {
         SharedSocket::bind(
-            ereport_port,
+            args.ereport_port,
             ereport::EreportHandler::default(),
             log.new(slog::o!("socket" => "ereport")),
         )

gateway-sp-comms/src/ereport.rs

@@ -30,8 +30,8 @@ use thiserror::Error;
 use tokio::sync::mpsc;
 use tokio::sync::oneshot;
 
-pub const SP_PORT: u16 = 0xDEAD;
-pub const MGS_PORT: u16 = 0xDEAF;
+pub const SP_PORT: u16 = 57005; // 0xDEAD
+pub const MGS_PORT: u16 = 57007; // 0xDEAF
 
 #[derive(Debug, Default)]
 pub struct EreportHandler {}
@@ -95,10 +95,19 @@ where
                 match self.request_ereports(RestartId(0), Ena(0), 0, None).await
                 {
                     Ok((restart_id, ereports)) => {
-                        debug_assert!(
-                            ereports.is_empty(),
-                            "we asked for limit 0..."
-                        );
+                        if !ereports.is_empty() {
+                            // The SP was told not to send ereports in this
+                            // request, just metadata. If it sends us ereports,
+                            // that could indicate a bug on the SP side, but we
+                            // assume the metadata is still valid regardless.
+                            warn!(
+                                self.log(),
+                                "received ereports in response to a request \
+                                 with limit 0, seems weird";
+                                "restart_id" => ?restart_id,
+                                "ereports" => ?ereports
+                            );
+                        }
                         debug!(
                             self.log(),
                             "received initial SP metadata";