Restructures LLDP service for safer state management #961

IDisposable · 2025-11-11T05:56:58Z

This targets your feat/lldp branch, and builds on PR #960 so merge that first or just this one

Refactors the LLDP (Link Layer Discovery Protocol) service to improve state management, resource handling, and lifecycle control for its receive and transmit functionalities.

Summary

LLDP Service Refactoring: Introduces a RunningState struct to encapsulate the operational state, context, logger, and packet resources for both LLDP RX and TX components independently. This change simplifies management, enhances resource cleanup, and clarifies concurrency handling by ensuring locks are properly released.
Configuration Updates: Handles LLDP mode options to ensure backward compatibility by remapping deprecated enabled, all, and basic modes to their modern equivalents in existing configurations using rx_only, tx_only, and rx_and_tx.
Improved Error Handling: Adds a global panic handler to ensure all uncaught panics are logged. Enhances error logging for network configuration operations, including hostname setting, DHCP client cleanup, and LLDP service initialization and updates.
UI Localization: Localizes the display of LLDP neighbor details in the user interface from PR Add localization of new elements #960

We were holding locks too long and doing more structure copying than optimal.

Emit more logging on errors Eliminated the redundant lldp.Start.

IDisposable · 2025-11-11T06:01:07Z

internal/confparser/confparser_test.go

 	IPv6Static *testIPv6StaticConfig `json:"ipv6_static,omitempty" required_if:"IPv6Mode=static"`

-	LLDPMode                null.String `json:"lldp_mode,omitempty" one_of:"disabled,basic,all" default:"basic"`
+	LLDPMode                null.String `json:"lldp_mode,omitempty" one_of:"disabled,enabled,rx_only,tx_only,rx_and_tx,basic,all" default:"rx_and_tx"`


enabled seems to be what we default to in the original code as tested against a factory reset

IDisposable · 2025-11-11T06:01:56Z

internal/lldp/lldp.go


 var defaultLogger = logging.GetSubsystemLogger("lldp")

+type RunningState struct {


Bundles up all the state for either Tx or Rx into a single object to ease ensuring proper running-state management.

IDisposable · 2025-11-11T06:02:41Z

internal/lldp/lldp.go

+type RunningState struct {
+	Enabled bool
+	Running bool
+	Logger  *zerolog.Logger


Passing the Logger and TPacket as pointers to ensure we don't accidentally miss mutations.

IDisposable · 2025-11-11T06:05:37Z

internal/lldp/lldp.go

-		l:                opts.Logger,
 		neighbors:        make(map[neighborCacheKey]Neighbor),
 		onChange:         opts.OnChange,
+		Rx:               &RunningState{},


We don't care about the initial enabled state because we're going to call the same SetRxAndTx method as would be used when changing options. This gives a single path by which Rx/Tx is Start/Stop.

Also means we can eliminate the Start() method entirely

IDisposable · 2025-11-11T06:06:13Z

internal/lldp/lldp.go

 		return nil
 	}

-	if err := l.setUpCapture(); err != nil {


Moved the setup inside startCapture()

IDisposable · 2025-11-11T06:07:31Z

internal/lldp/lldp.go

 func (l *LLDP) SetAdvertiseOptions(opts *AdvertiseOptions) error {
 	l.mu.Lock()
-	txRunning := l.txRunning
+	logger := l.l


Since this gets called because of change in the advertising options, we can't use either the Rx or Tx loggers, so use the top-level LLDP logger.

IDisposable · 2025-11-11T06:08:36Z

internal/lldp/lldp.go

 	l.mu.Lock()
-	l.enableRx = rx
-	l.enableTx = tx
+	l.Rx.Enabled = rx


This is now executed both when things change and one time at device startup

IDisposable · 2025-11-11T06:11:22Z

internal/lldp/rx.go

 }

-func (l *LLDP) setUpCapture() error {
-	l.mu.Lock()


This method is now executed while already holding the lock inside prepareCapture() and it has setup all the Rx logger and rx TPacket before we get here.

IDisposable · 2025-11-11T06:11:52Z

internal/lldp/rx.go

-	}
-	logger.Info().Msg("created TPacketRx")
-
+func (l *LLDP) setUpPacketSourceUnderLock(tPacket *afpacket.TPacket, logger *zerolog.Logger) (*gopacket.PacketSource, error) {


This now is only tasked with setting up the BPF filter and packet source.

IDisposable · 2025-11-11T06:12:51Z

internal/lldp/rx.go

-	if l.pktSourceRx == nil || l.rxCtx == nil {
+func (l *LLDP) doCapture() {
+	l.mu.Lock()
+	l.Rx.Running = true


doCapture() now manages the Rx running flag like we did in Tx.

IDisposable · 2025-11-11T06:13:28Z

internal/lldp/rx.go

+	}()
+
+	if l.pktSourceRx == nil || ctx == nil {
 		logger.Error().Msg("packet source or RX context not initialized")


I don't think this clause could ever happen, but left it in just in case.

IDisposable · 2025-11-11T06:14:05Z

internal/lldp/rx.go

-			if err == io.EOF || err == io.ErrUnexpectedEOF ||
-				err == io.ErrNoProgress || err == io.ErrClosedPipe || err == io.ErrShortBuffer ||
-				err == syscall.EBADF ||
+			if errors.Is(err, io.EOF) ||


The is the preferred idiom since 1.3 because it can recognize wrapped errors

IDisposable · 2025-11-11T06:15:22Z

internal/lldp/rx.go

 }

-func (l *LLDP) startCapture() error {
+func (l *LLDP) prepareCapture() (bool, error) {


prepareCapture() does all the setup and returns true if the caller needs to call doCapture() (e.g. run the Rx loop).

IDisposable · 2025-11-11T06:15:54Z

internal/lldp/rx.go

-	if l.rxRunning {
-		return nil // Already running
+	if l.Rx.Running {
+		l.l.Debug().Msg("LLDP receiver already running")


... we haven't created the Rx logger yet, use the LLDP level one.

IDisposable · 2025-11-11T06:17:21Z

internal/lldp/rx.go

+	runningState := &RunningState{Enabled: true, Logger: &logger}
+	runningState.Ctx, runningState.Cancel = context.WithCancel(context.Background())
+	runningState.TPacket = tPacket
+	l.Rx = runningState


monotonic switch the new running Rx state...

IDisposable · 2025-11-11T06:19:47Z

internal/lldp/rx.go

-		l.tPacketRx.Close()
-		l.tPacketRx = nil
-	}
+	l.Rx.Running = false


Eagerly clear the running flag because we're about to tear everything down in reverse order.

IDisposable · 2025-11-11T06:21:23Z

internal/lldp/tx.go

 )

-func (l *LLDP) toPayloadValues() []layers.LinkLayerDiscoveryValue {
+func (l *LLDP) toPayloadValues(opts *AdvertiseOptions) []layers.LinkLayerDiscoveryValue {


Passing in the options we grabbed under read-lock in sendTxPackets()

IDisposable · 2025-11-11T06:23:32Z

internal/lldp/tx.go

-	l.tPacketTx = tPacketTx

+	logger.Info().Msg("created TPacket instance for sending LLDP packets")
+	l.Tx = txState


monotonically swap Tx running state

IDisposable · 2025-11-11T06:26:11Z

internal/lldp/tx.go


-	if cancel != nil {
-		cancel()
+	if previousCancel != nil {


Renamed for clarification that this forces the shut-down of the previously running Tx loop

IDisposable · 2025-11-11T06:27:56Z

internal/lldp/tx.go

-	txCancel := l.txCancel
-	l.txRunning = false
+	txCancel := l.Tx.Cancel
+	l.Tx.Running = false


techinically we should probably let the defer code in doSendPeriodically() clear this flag, but an early teardown indication isn't going to hurt us here.

IDisposable · 2025-11-11T06:28:57Z

internal/network/types/config.go

 	IPv6Static *IPv6StaticConfig `json:"ipv6_static,omitempty" required_if:"IPv6Mode=static"`

-	LLDPMode                null.String `json:"lldp_mode,omitempty" one_of:"disabled,rx_only,tx_only,rx_and_tx,basic,all" default:"rx_and_tx"`
+	LLDPMode                null.String `json:"lldp_mode,omitempty" one_of:"disabled,enabled,rx_only,tx_only,rx_and_tx,basic,all" default:"rx_and_tx"`


Seems like the factory-reset defaults to enabled not any of the other options, so declare it as valid to avoid config validation errors.

IDisposable · 2025-11-11T06:30:08Z

pkg/nmlite/link/manager.go

 	// add remaining expected default routes
 	for _, gateway := range expected {
-		nm.logger.Warn().Str("gateway", gateway.String()).Msg("adding default route")
+		nm.logger.Info().Str("gateway", gateway.String()).Msg("adding default route")


Could be .Debug() if we wanted less logging.

IDisposable · 2025-11-11T06:31:05Z

config.go

 		loadedConfig.KeyboardLayout = "en-US"
 	}

+	// fixup old lldp modes enabled and all are now rx_and_tx


Do a load-time migration of the configuration from the old legacy values to what we interpret them as now.

IDisposable · 2025-11-11T06:32:51Z

main.go


 func Main() {
 	logger.Log().Msg("JetKVM Starting Up")
+


Setup a top-level panic handler to catch and log unhandled panics so we at least get a log message. We might want to add an reboot() call here to see if we can recover?

IDisposable · 2025-11-11T06:33:20Z

network.go

 	networkLogger.Info().Interface("networkConfig", nc).Str("hostname", nc.Hostname.String).Str("domain", nc.Domain.String).Msg("initializing network manager")
-	_ = setHostname(nm, nc.Hostname.String, nc.Domain.String)
+
+	if err := setHostname(nm, nc.Hostname.String, nc.Domain.String); err != nil {


Seems bad manners to ignore these errors, so emit them to the log at least.

IDisposable · 2025-11-11T06:34:11Z

network.go

 	})
-	if err := lldpService.Start(); err != nil {
-		networkLogger.Error().Err(err).Msg("failed to start LLDP service")
+


Instead of a distinct Start() method, we can just leverage the SetRxAndTx() function to manage the Rx and Tx loops.

IDisposable · 2025-11-11T06:35:43Z

network.go


 	if err := lldpService.SetRxAndTx(nc.ShouldEnableLLDPReceive(), nc.ShouldEnableLLDPTransmit()); err != nil {
-		networkLogger.Error().Err(err).Msg("failed to set LLDP RX and TX")
+		networkLogger.Error().Err(err).Msg("failed to update LLDP RX and TX")


Disambiguates the "update" from the "initialize" path in network.go

IDisposable · 2025-11-11T06:37:30Z

I tested this with reboots in every setting (none/rx-only/tx-only/both) and flipping around through the options and it looks like it works correctly without any panics.

IDisposable · 2025-11-11T18:04:08Z

@ym, I left this branch running on my personal JetKVM overnight and came back to a logo-only screen and unresponsive to the local IP. Not sure what happened, and in the process of trying to grab logs I broke off the HDMI connector (Doh!!) and now that guy is toast. Sorry. I have another so continuing on with coding/testing... but wanted to warn you that this version might have an issue I'm unaware of the cause. I did have the HDMI sleep enabled, so could be something there unrelated to this change...

IDisposable added 6 commits November 10, 2025 18:56

Sort en.json with npm run i18n:resort

fe74d8f

Localize LLDP card

af8bfc7

Restructure to ensure we're releasing locks

b6c1e49

We were holding locks too long and doing more structure copying than optimal.

Add a global panic handler to ensure we log panics

96c005b

Create lldpService then start/stop Rx/Tx

43c1468

Emit more logging on errors Eliminated the redundant lldp.Start.

Read rx logger under lock.

7689f2e

IDisposable requested a review from ym November 11, 2025 05:58

IDisposable commented Nov 11, 2025

View reviewed changes

internal/lldp/lldp.go

return nil

}

if err := l.setUpCapture(); err != nil {

Copy link

Contributor Author

IDisposable Nov 11, 2025

Choose a reason for hiding this comment

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

Moved the setup inside startCapture()

IDisposable commented Nov 11, 2025

View reviewed changes

IDisposable mentioned this pull request Nov 11, 2025

feat: lldp #948

Open

ym approved these changes Nov 11, 2025

View reviewed changes

ym merged commit 7ea59ba into jetkvm:feat/lldp Nov 11, 2025
31 checks passed

IDisposable deleted the feat/lldp-restructure branch November 11, 2025 16:41

IDisposable mentioned this pull request Nov 11, 2025

Add localization of new elements #960

Closed


		var defaultLogger = logging.GetSubsystemLogger("lldp")

		type RunningState struct {

Restructures LLDP service for safer state management #961

Restructures LLDP service for safer state management #961

Uh oh!

Conversation

IDisposable commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IDisposable Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IDisposable commented Nov 11, 2025

Uh oh!

Uh oh!

IDisposable commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IDisposable commented Nov 11, 2025 •

edited

Loading

IDisposable Nov 11, 2025 •

edited

Loading