feat: lldp #948
feat: lldp #948
Conversation
|
I can't seem to fix the segfault issue when closing PacketSource, and I can see that other people are having the same issue with GoPacket. I will move the Rx process to a separate process to avoid this issue. |
I would hate for us to spin up a process just for that! Can I take a gander and see if I can figure it out? There are some mutex issues I noted, but I haven't even tries to run this code... I might be able to find the issue once I do? Can you give me the steps to make it segfault and/or the stack trace? |
internal/network/types/config.go
Outdated
| IPv6Static *IPv6StaticConfig `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,rx_only,tx_only,rx_and_tx,basic,all" default:"rx_and_tx"` |
There was a problem hiding this comment.
Should we remove ,basic,all here and shouldn't the default now be rx_only per comment about privacy concerns on line 57?
If we're now only taking the list of options in stores.ts line 64, do we need a config conversion that morphs basic and all to to rx_only and rx_and_tx (respectively), or do we not care, since the LLDP was never really implemented?
There was a problem hiding this comment.
It's kept here for backwards compatibility, if the value doesn't exist in one_off, an error will be thrown :-(
There was a problem hiding this comment.
Should we morph it over as we read it in (like I once did for en_US vs en-US?)
I did that in #961
I kinda fixed it, but the solution is quite dirty. If you come up with a cleaner approach, go for it. |
Looks fine by me :) |
|
nvm, it's stupid React key issue :-) |
internal/lldp/rx.go
Outdated
| l.rxCtx, l.rxCancel = context.WithCancel(context.Background()) | ||
| l.rxRunning = true | ||
|
|
||
| go l.doCapture(&logger) |
There was a problem hiding this comment.
Doesn't this mean that the go routine is running inside the lock from line 140? Shouldn't we put all the "setup" work in a distinct function and run that all under a defer unlock, call that setup, and then start the go routine?
There was a problem hiding this comment.
I created a PR #961 targeting your feat/lldp branch with a safer implementation of this code (in tx.go, rx.go, lldp.go) to make the locks and pointer ownership clearer.
internal/lldp/tx.go
Outdated
| } | ||
|
|
||
| if cancel != nil { | ||
| cancel() |
There was a problem hiding this comment.
Note to self: This cancels any previously running Tx loop. This code is idiomatically correct.
internal/network/types/config.go
Outdated
| IPv6Static *IPv6StaticConfig `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,rx_only,tx_only,rx_and_tx,basic,all" default:"rx_and_tx"` |
There was a problem hiding this comment.
Should we morph it over as we read it in (like I once did for en_US vs en-US?)
I did that in #961
internal/lldp/rx.go
Outdated
| l.rxCtx, l.rxCancel = context.WithCancel(context.Background()) | ||
| l.rxRunning = true | ||
|
|
||
| go l.doCapture(&logger) |
There was a problem hiding this comment.
I created a PR #961 targeting your feat/lldp branch with a safer implementation of this code (in tx.go, rx.go, lldp.go) to make the locks and pointer ownership clearer.
internal/lldp/tx.go
Outdated
| } | ||
|
|
||
| if cancel != nil { | ||
| cancel() |
e.g.