Conversation
a1802b8 to
9f7f3e0
Compare
Enable bidirectional I/O for serial console access: - Use Console() for stdin/stdout instead of Print() - Forward client input to serial port in realtime - Set terminal to raw input mode (no echo, no line buffering) - Filter terminal escape sequences from serial I/O - Add flush timeout for prompts without newlines Signed-off-by: llogen <christoph.lange@blindspot.software>
9f7f3e0 to
87a4da9
Compare
Signed-off-by: llogen <christoph.lange@blindspot.software>
e1567e2 to
af2dd18
Compare
176f7c1 to
02f20d0
Compare
Signed-off-by: llogen <christoph.lange@blindspot.software>
Signed-off-by: llogen <christoph.lange@blindspot.software>
02f20d0 to
a68f4a1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/module/serial/serial.go
Outdated
| expect *regexp.Regexp // expect is a pattern to match against the serial output. | ||
| timeout time.Duration // timeout is the maximum time to wait for the expect pattern to match. | ||
| csiRemainder []byte // csiRemainder holds an incomplete CSI sequence carried over across buffer reads. |
There was a problem hiding this comment.
These fields store per-run state, but module instances are created once from YAML config and reused across multiple RPC runs. evalArgs currently only compiles s.expect when a pattern is provided and never clears it when no pattern is provided, so a previous run's expect regex can carry over into later runs unexpectedly. Also consider resetting csiRemainder at the start of Run() to avoid cross-run leakage if a run ends mid-escape-sequence.
| // Multiple calls are idempotent; subsequent calls return the already initialized session and channel. | ||
| // | ||
| //nolint:ireturn // returning interface module.Session is intentional for abstraction boundary | ||
|
|
There was a problem hiding this comment.
Start returns module.Session (an interface type) and this repository enables the ireturn linter. Removing the //nolint:ireturn suppression here will likely cause golangci-lint to fail unless Session is added to the ireturn.allow list. Either restore the nolint directive or update the linter config to permit returning module.Session.
| //nolint:ireturn // Start intentionally returns module.Session (an interface) as part of the public API. |
| // Close stdinCh on exit so ChanReader.Read returns io.EOF and any module | ||
| // goroutine blocked on stdin (e.g. the serial inner goroutine) unblocks cleanly. | ||
| defer close(s.stdinCh) |
There was a problem hiding this comment.
With raw-mode stdin forwarding, fromClientWorker will now receive and forward very frequent stdin chunks (often single keystrokes). Logging the stdin payload (currently done later in this function) will both flood logs and potentially record sensitive input (e.g., passwords typed into a serial console). Consider removing stdin content logging entirely, or only logging metadata like byte length behind a debug flag.
| readResultCh <- readResult{data: cp, err: err} | ||
|
|
There was a problem hiding this comment.
The stdin forwarding uses an inner goroutine that always sends a readResult into readResultCh. If Run() exits (done is closed) while the inner goroutine is blocked on readResultCh <- ..., it can deadlock/leak forever because the outer goroutine has stopped receiving. Consider selecting on done when sending to readResultCh (or closing the channel / using a single goroutine) so shutdown can't strand the sender.
| readResultCh <- readResult{data: cp, err: err} | |
| select { | |
| case <-done: | |
| return | |
| case readResultCh <- readResult{data: cp, err: err}: | |
| } |
| if lineBuffer.Len() > 0 { | ||
| flushGen++ | ||
| thisGen := flushGen | ||
|
|
||
| time.AfterFunc(flushTimeout, func() { | ||
| // Phase 1: drain the buffer under the lock (fast, no I/O). |
There was a problem hiding this comment.
A new time.AfterFunc is scheduled on every read while lineBuffer is non-empty. With high-volume output that doesn't include newlines, this can create an unbounded number of timers/goroutines that mostly no-op via flushGen, but still consume CPU/memory and add scheduler pressure. Prefer a single reusable time.Timer (reset on activity) or a dedicated flusher goroutine that coalesces flush requests.
Allow automated serial interaction by passing pattern/response pairs as arguments. The module sends each response to the serial port when its pattern matches, then exits after the last pair completes. Response strings support C-style escape sequences (\n, \r, \t, \\, \xHH). Signed-off-by: llogen <christoph.lange@blindspot.software>
Signed-off-by: llogen <christoph.lange@blindspot.software>
Enable bidirectional I/O for serial console access: