Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ pub use client::{
};
pub use e2e::{E2ECheckStatus, E2EKey, E2EProfile};
#[cfg(feature = "server")]
pub use server::{Server, ServerDeps, SubscriptionHandle};
pub use server::{Server, ServerDeps, ServerHandles, SubscriptionHandle};
#[cfg(any(feature = "client-tokio", feature = "server-tokio"))]
pub use tokio_transport::{TokioChannels, TokioSocket, TokioSpawner, TokioTimer, TokioTransport};
#[cfg(feature = "bare_metal")]
Expand Down
155 changes: 155 additions & 0 deletions src/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,63 @@ where
pub subscriptions: S,
}

/// Bundle of pre-built dependencies + storage handles for
/// [`Server::new_with_handles`] / [`Server::new_passive_with_handles`].
///
/// Variant of [`ServerDeps`] for callers who have already bound
/// their sockets externally and assembled storage handles
/// themselves — the bare-metal-no-alloc path. Each
/// `Wrappable*Handle`-using constructor on the alloc path
/// (`Server::new_with_deps`, `Server::new_passive_with_deps`) has a
/// counterpart here that takes pre-built handles directly,
/// skipping the internal `wrap` step. That lets a no-alloc consumer
/// supply `StaticSocketHandle<EmbassyNetSocket>` /
/// `&'static SdStateManager` / `&'static EventPublisher<...>`
/// instances they materialized via their preferred static-storage
/// pattern.
///
/// All eight fields are public so the struct can be assembled
/// inline.
pub struct ServerHandles<F, Tm, R, S, H, Hsd, Hep>
where
F: TransportFactory + 'static,
Tm: Timer,
R: E2ERegistryHandle,
S: SubscriptionHandle,
H: SocketHandle<Socket = F::Socket>,
Hsd: SdStateHandle,
Hep: EventPublisherHandle<R, S, H>,
{
/// Transport factory. Retained on the `Server` for any
/// post-construction state the backend needs to keep alive
/// (e.g., embassy-net `Stack` handle); the new-with-handles
/// constructor does NOT call `factory.bind()`.
pub factory: F,
/// Async sleep primitive used by the announcement loop's
/// 1-second tick.
pub timer: Tm,
/// Shared E2E registry handle for runtime E2E configuration.
pub e2e_registry: R,
/// Shared subscription manager handle.
pub subscriptions: S,
/// Pre-built unicast socket handle. Caller has already bound
/// the underlying socket to the desired interface + port.
pub unicast_socket: H,
/// Pre-built SD socket handle. For active servers, caller has
/// bound to the SD multicast port (30490) and joined the SD
/// multicast group; for passive servers, this is whatever
/// placeholder socket the caller chose (will not be driven).
pub sd_socket: H,
/// Pre-built SD-state handle (`&'static SdStateManager` for
/// no-alloc, `Arc<SdStateManager>` for alloc).
pub sd_state: Hsd,
/// Pre-built `EventPublisher` handle. For std users this is
/// typically `Arc<EventPublisher::new(subscriptions, unicast,
/// e2e)>`; for no-alloc, a `&'static EventPublisher<...>`
/// declared externally.
pub publisher: Hep,
}

/// SOME/IP Server that can offer services and publish events.
///
/// Generic over the four pluggable infrastructure types bundled in
Expand Down Expand Up @@ -472,6 +529,104 @@ where
Hsd: SdStateHandle,
Hep: EventPublisherHandle<R, S, H>,
{
/// Construct a `Server` from pre-built dependencies + storage
/// handles. The bare-metal-no-alloc counterpart to
/// [`Self::new_with_deps`].
///
/// Unlike `new_with_deps`, this constructor does NOT call
/// `factory.bind(...)` and does NOT join any multicast group.
/// The caller has already bound their unicast and SD sockets
/// (typically against an externally-managed UDP stack — lwIP,
/// vendor IP, etc.) and joined the SOME/IP-SD multicast group
/// (`224.0.23.0`) on the SD socket externally. The caller has
/// also assembled the `EventPublisher` and `SdStateManager`
/// handles into whatever shared-storage their target uses
/// (`Arc<...>` on alloc, `&'static ...` on no-alloc).
///
/// `config.local_port` is back-filled from
/// `unicast_socket.local_addr()?.port()` so SD offers and
/// event publishers advertise the actual bound port.
///
/// # Errors
///
/// Returns an error if querying `unicast_socket.local_addr()`
/// fails on the underlying transport.
pub fn new_with_handles(
deps: ServerHandles<F, Tm, R, S, H, Hsd, Hep>,
mut config: ServerConfig,
) -> Result<Self, Error> {
Comment on lines +554 to +557

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are extensive constructor-focused tests in this file, but the new public constructor new_with_handles isn’t covered. Adding a unit test that builds a ServerHandles with a minimal TransportSocket stub (local_addr returning a known port) would help ensure config.local_port back-fill and the resulting is_passive flag behave as intended.

Copilot uses AI. Check for mistakes.
let bound_port = deps.unicast_socket.socket().local_addr()?.port();
config.local_port = bound_port;
Comment on lines +556 to +559

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_with_handles overwrites config.local_port unconditionally with the socket’s bound port. This differs from new_with_deps (which only back-fills when the caller passed local_port = 0) and can silently mask a misconfiguration where the caller thinks they bound a specific port but actually didn’t. Consider either (a) only overwriting when config.local_port == 0, or (b) returning Error::InvalidUsage(...) / logging a warning if config.local_port != 0 && config.local_port != bound_port.

Copilot uses AI. Check for mistakes.
tracing::info!(
"Server (handles) bound to {}:{} for service 0x{:04X}",
config.interface,
bound_port,
config.service_id
);
Comment on lines +558 to +565

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_with_handles uses config.interface later when constructing IpV4Endpoint options (e.g. in send_unicast_offer), but here it only back-fills the port from unicast_socket.local_addr() and does not validate that the socket is actually bound to config.interface. If the caller bound to a different IP (or 0.0.0.0), the server may advertise the wrong endpoint address. Consider validating local_addr.ip() against config.interface (with an explicit exception if the socket is bound to INADDR_ANY) and returning Error::InvalidUsage(...) or at least emitting a warning on mismatch.

Copilot uses AI. Check for mistakes.

Ok(Self {
config,
unicast_socket: deps.unicast_socket,
sd_socket: deps.sd_socket,
Comment on lines +567 to +570

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an active server, the SD socket is expected to be bound to the SOME/IP-SD port (30490) so outgoing announcements and SubscribeAck/Nack responses have the correct source port. new_with_handles currently trusts the caller and never checks sd_socket.socket().local_addr()?.port() == sd::MULTICAST_PORT; if it’s wrong, SD traffic will be emitted from an unexpected port and many peers will ignore it. Consider validating the SD socket port here (and returning Error::InvalidUsage(...) on mismatch) to fail fast on misconfiguration.

Copilot uses AI. Check for mistakes.
subscriptions: deps.subscriptions,
publisher: deps.publisher,
sd_state: deps.sd_state,
e2e_registry: deps.e2e_registry,
factory: deps.factory,
timer: deps.timer,
is_passive: false,
announcement_loop_started: AtomicBool::new(false),
})
}

/// Passive-server counterpart to [`Self::new_with_handles`].
///
/// Same shape; the resulting server is marked
/// `is_passive = true` so [`Self::announcement_loop`] /
/// [`Self::announcement_loop_local`] / [`Self::run`] /
/// [`Self::run_with_buffers`] return
/// `Err(Error::InvalidUsage(...))` rather than driving the SD
/// loop. The caller is expected to handle SD externally
/// (typically via a `Client::sd_announcements_loop` on the
/// same host).
///
/// The `sd_socket` field is retained but never driven; pass
/// any pre-built handle the caller can spare (a placeholder
/// socket bound to an ephemeral port is fine, mirroring
/// `Server::new_passive_with_deps`).
///
/// # Errors
///
/// Returns an error if querying `unicast_socket.local_addr()`
/// fails on the underlying transport.
pub fn new_passive_with_handles(
deps: ServerHandles<F, Tm, R, S, H, Hsd, Hep>,
mut config: ServerConfig,
) -> Result<Self, Error> {
let bound_port = deps.unicast_socket.socket().local_addr()?.port();
config.local_port = bound_port;

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_passive_with_handles overwrites config.local_port unconditionally with the socket’s bound port. For consistency with new_passive_with_deps (and to avoid masking mismatches), consider only back-filling when config.local_port == 0, or returning Error::InvalidUsage(...) / logging a warning if a non-zero config.local_port does not match the socket’s bound port.

Suggested change
config.local_port = bound_port;
if config.local_port == 0 {
config.local_port = bound_port;
} else if config.local_port != bound_port {
return Err(Error::InvalidUsage(format!(
"new_passive_with_handles local_port mismatch: config.local_port={} but unicast socket is bound to {}",
config.local_port, bound_port
)));
}

Copilot uses AI. Check for mistakes.
tracing::info!(
"Passive server (handles) bound to {}:{} for service 0x{:04X}",
config.interface,
bound_port,
config.service_id
);
Comment on lines +606 to +613

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as new_with_handles: new_passive_with_handles relies on config.interface for SD endpoint construction elsewhere, but doesn’t validate it against unicast_socket.local_addr().ip(). A mismatch can lead to incorrect endpoint IP being advertised in OfferService responses. Consider validating (or warning) on interface mismatch, with an explicit exception for sockets bound to 0.0.0.0 if you want to allow INADDR_ANY binds.

Copilot uses AI. Check for mistakes.

Ok(Self {
config,
unicast_socket: deps.unicast_socket,
sd_socket: deps.sd_socket,
subscriptions: deps.subscriptions,
publisher: deps.publisher,
sd_state: deps.sd_state,
e2e_registry: deps.e2e_registry,
factory: deps.factory,
timer: deps.timer,
is_passive: true,
announcement_loop_started: AtomicBool::new(false),
})
}

/// Build the periodic-SD-announcement future.
///
/// Returns a future that sends an `OfferService` message to the SD
Expand Down