docs: Add architecture guide for component composition#149
Conversation
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new architecture guide to document how Magma’s components compose across OSI layers, how packets flow through the stack, and how platform backends integrate via common abstractions—aimed at helping contributors understand where new code fits.
Changes:
- Introduces
docs/ARCHITECTURE.mddescribing OSI-layer mapping, packet flow, and backend integration. - Documents
IPacketTransmitter/IPacketReceivercontracts and delegate-based processing inMagma.Network. - Adds diagrams (Mermaid + ASCII) and extension guidelines for adding protocols/backends.
|
|
||
| **Key Points:** | ||
| - **Buffer Ownership**: Caller gets buffer from transmitter, fills it, returns it | ||
| - **Async Send**: `SendBuffer()` returns immediately; actual transmission is async |
There was a problem hiding this comment.
This describes SendBuffer() as returning immediately with "actual transmission is async". At least one implementation (WinTunTransitter.SendBuffer) awaits the underlying write and only completes when the send finishes. Consider rephrasing to the more accurate contract: SendBuffer is asynchronous (returns a Task) and completion semantics depend on the backend implementation.
| - **Async Send**: `SendBuffer()` returns immediately; actual transmission is async | |
| - **Async Send Contract**: `SendBuffer()` is asynchronous (returns a `Task`); whether the task completes after queuing or after the underlying write/transmission finishes depends on the backend implementation |
|
|
||
| 1. **Bounded Access**: All buffer accesses are bounds-checked via `Span<T>` | ||
| 2. **Struct Layout**: `StructLayout` attributes guarantee correct alignment | ||
| 3. **No Pointer Arithmetic**: Use `Span<T>` methods instead of raw pointers |
There was a problem hiding this comment.
The "Unsafe Code Safety" rules claim "No Pointer Arithmetic", but the repo does use pointer arithmetic in places (e.g., WinTun memory pool uses byte* offsetting). Consider tightening this to what is actually enforced: minimize pointer arithmetic and confine it to well-audited buffer/memory-manager code, while preferring Span<T> operations for parsing/processing.
| 3. **No Pointer Arithmetic**: Use `Span<T>` methods instead of raw pointers | |
| 3. **Minimize Pointer Arithmetic**: Confine raw pointer arithmetic to well-audited buffer/memory-manager code; prefer `Span<T>` operations for parsing and processing |
|
|
||
| #### Magma.Transport.Tcp (Transport Layer) | ||
| Full TCP implementation: | ||
| - **`TcpHeader` struct**: 20-byte base TCP header |
There was a problem hiding this comment.
This section references a TcpHeader struct as the 20-byte base TCP header, but the codebase's base TCP header type is Magma.Network.Header.Tcp (with TcpHeaderWithOptions layering options on top). Updating the type name here will make it easier for contributors to locate the correct implementation.
| - **`TcpHeader` struct**: 20-byte base TCP header | |
| - **`Magma.Network.Header.Tcp` struct**: 20-byte base TCP header |
| // Packet consumed, dispose and return null | ||
| input.Dispose(); | ||
| return default; | ||
| } |
There was a problem hiding this comment.
The example implementation comment and return value here say "return null" to indicate a consumed packet, but TryConsume<T> returns T where T : IMemoryOwner<byte> and callers check default/empty rather than null. Please change this example to return default (and optionally explain how "empty" is detected for struct wrappers).
| **Contract Rules:** | ||
| - `TryConsume()` returns `null` if packet was consumed | ||
| - `TryConsume()` returns `input` if packet should be passed to OS | ||
| - Caller must dispose `input` if it returns non-null | ||
| - `FlushPendingAcks()` is called periodically to send deferred transmissions |
There was a problem hiding this comment.
IPacketReceiver.TryConsume<T> does not consistently use null to signal consumption. The interface returns T where T : IMemoryOwner<byte> (not constrained to reference types), and current callers check for default/empty (e.g., NetMap checks .IsEmpty, WinTun checks == default). Please update the contract description and rules here to describe default(T) (or an empty wrapper) as the consumed signal, and clarify the ownership/return responsibility for the non-consumed case accordingly.
| 2. **Parse IP** → Dispatch based on protocol number | ||
| 3. **Parse Transport** → Deliver to application | ||
|
|
||
| If any layer returns `false`, the packet is passed to the host OS network stack. |
There was a problem hiding this comment.
The statement that "If any layer returns false, the packet is passed to the host OS network stack" is not accurate for malformed frames. In Magma.Network.PacketReceiver.TryConsume, an Ethernet parse failure sets result = true (consume/drop) rather than passing through. Consider rewording to distinguish: delegates returning false => passthrough, but invalid/unparseable frames are consumed/dropped.
| If any layer returns `false`, the packet is passed to the host OS network stack. | |
| If parsing succeeds and a delegate at any layer returns `false`, the packet is passed to the host OS network stack; if a frame cannot be parsed (for example, an invalid Ethernet header), `PacketReceiver` consumes/drops it instead of passing it through. |
| │ │ WinTun Session │ │ | ||
| │ │ │ │ | ||
| │ │ Receive Thread ───calls──→ IPacketReceiver │ │ | ||
| │ │ │ │ │ | ||
| │ │ │ (polls WintunReceivePacket) │ │ | ||
| │ │ │ │ │ | ||
| │ │ WinTunTransmitter ──implements──→ IPacketTransmitter │ │ | ||
| │ │ │ │ │ | ||
| │ │ │ (calls WintunAllocateSendPacket/SendPacket) │ │ | ||
| │ │ │ │ | ||
| │ └──────────────────────────────────────────────────────────┘ │ | ||
| │ │ | ||
| │ WinTun Driver (kernel mode) │ | ||
| │ ┌────────────────────────────────────────────────────────┐ │ | ||
| │ │ Virtual Network Adapter │ │ |
There was a problem hiding this comment.
The WinTun backend diagram text references APIs like WintunReceivePacket / WintunAllocateSendPacket, but the current Windows backend code opens a *.tap device via CreateFile and uses a FileStream to read/write. Please update this section to match the actual implementation (TAP device + async file I/O) or rename the backend if it is meant to target the WireGuard Wintun API.
| │ │ WinTun Session │ │ | |
| │ │ │ │ | |
| │ │ Receive Thread ───calls──→ IPacketReceiver │ │ | |
| │ │ │ │ │ | |
| │ │ │ (polls WintunReceivePacket) │ │ | |
| │ │ │ │ │ | |
| │ │ WinTunTransmitter ──implements──→ IPacketTransmitter │ │ | |
| │ │ │ │ │ | |
| │ │ │ (calls WintunAllocateSendPacket/SendPacket) │ │ | |
| │ │ │ │ | |
| │ └──────────────────────────────────────────────────────────┘ │ | |
| │ │ | |
| │ WinTun Driver (kernel mode) │ | |
| │ ┌────────────────────────────────────────────────────────┐ │ | |
| │ │ Virtual Network Adapter │ │ | |
| │ │ WinTun/TAP Session │ │ | |
| │ │ │ │ | |
| │ │ Receive Thread ───calls──→ IPacketReceiver │ │ | |
| │ │ │ │ │ | |
| │ │ │ (async reads from TAP device FileStream) │ │ | |
| │ │ │ │ │ | |
| │ │ WinTunTransmitter ──implements──→ IPacketTransmitter │ │ | |
| │ │ │ │ │ | |
| │ │ │ (async writes to TAP device FileStream) │ │ | |
| │ │ │ │ | |
| │ └──────────────────────────────────────────────────────────┘ │ | |
| │ │ | |
| │ Windows TAP Driver (kernel mode) │ | |
| │ ┌────────────────────────────────────────────────────────┐ │ | |
| │ │ TAP Virtual Network Adapter │ │ |
| - **Zero-Copy**: The packet buffer stays in shared memory throughout | ||
| - **Type-Punning**: Headers are overlaid on byte spans using `Unsafe.As<,>()` | ||
| - **Early Rejection**: Invalid packets are rejected as early as possible | ||
| - **Passthrough**: Unhandled packets return non-null from `TryConsume()` to pass to host OS |
There was a problem hiding this comment.
This section says "Unhandled packets return non-null from TryConsume()". In this codebase, consumption is signaled by returning default(T)/empty (not necessarily null), and passthrough is signaled by returning a non-default owner (e.g., NetMap checks .IsEmpty, WinTun checks == default). Please update wording here to avoid implying reference-type null semantics.
| - **Passthrough**: Unhandled packets return non-null from `TryConsume()` to pass to host OS | |
| - **Passthrough**: Unhandled packets return a non-default owner from `TryConsume()` (default/empty means consumed; non-default passes the buffer to the host OS) |
| While Magma uses `unsafe` code extensively, it follows strict safety rules: | ||
|
|
||
| 1. **Bounded Access**: All buffer accesses are bounds-checked via `Span<T>` | ||
| 2. **Struct Layout**: `StructLayout` attributes guarantee correct alignment |
There was a problem hiding this comment.
StructLayout (especially with Pack = 1) guarantees field layout/padding, but it does not guarantee “correct alignment” (it typically forces 1-byte alignment, which can lead to unaligned reads). Consider rewording this bullet to focus on wire-compat layout rather than alignment guarantees.
| 2. **Struct Layout**: `StructLayout` attributes guarantee correct alignment | |
| 2. **Struct Layout**: `StructLayout` attributes ensure deterministic field layout that matches the wire format (not CPU alignment) |
| 1. **Platform Backend Allocates**: Buffers come from shared memory region (mmap, UMEM) | ||
| 2. **Wrapped in IMemoryOwner**: Buffer wrapped with ownership tracking | ||
| 3. **Passed Up Stack**: Each layer parses headers, slices to payload | ||
| 4. **Single Owner**: Only one component "owns" buffer at a time | ||
| 5. **Dispose to Release**: Calling `Dispose()` returns buffer to pool |
There was a problem hiding this comment.
The buffer ownership model says platform buffers come from a shared memory region (mmap/UMEM). That’s true for NetMap/AF_XDP, but the Windows backend currently allocates its pool via Marshal.AllocHGlobal and uses TAP file I/O (not a shared UMEM-style region). Consider qualifying this point as backend-dependent to avoid overstating zero-copy/shared-memory guarantees.
| 1. **Platform Backend Allocates**: Buffers come from shared memory region (mmap, UMEM) | |
| 2. **Wrapped in IMemoryOwner**: Buffer wrapped with ownership tracking | |
| 3. **Passed Up Stack**: Each layer parses headers, slices to payload | |
| 4. **Single Owner**: Only one component "owns" buffer at a time | |
| 5. **Dispose to Release**: Calling `Dispose()` returns buffer to pool | |
| 1. **Platform Backend Allocates**: AF_XDP/NetMap use a shared memory region (mmap/UMEM); WinTun uses an unmanaged buffer pool (e.g. `Marshal.AllocHGlobal`) with TAP file I/O | |
| 2. **Wrapped in IMemoryOwner**: Buffers are exposed to Magma via `IMemoryOwner<byte>` (or equivalent) for ownership tracking | |
| 3. **Passed Up Stack**: Each layer parses headers, slices to payload | |
| 4. **Single Owner**: Only one component "owns" buffer at a time | |
| 5. **Dispose to Release**: Calling `Dispose()` returns buffer to the backend-specific pool |
|
@copilot apply changes based on the comments in this thread |
Contributors lack visibility into how Magma components compose, blocking understanding of where new code integrates.
Changes
Core Documentation (
docs/ARCHITECTURE.md)IPacketTransmitter/IPacketReceivercontract and implementation patternsMagma.Networkuses delegates for extensible protocol processingKey Concepts Explained
Packet processing chain:
Platform abstraction:
Includes buffer ownership model, header parsing patterns (
Unsafe.As<,>()type-punning), and backend comparison matrix.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.