Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor monitor and implment devd monitoring for freebsd #41

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

wardn
Copy link
Contributor

@wardn wardn commented Feb 12, 2020

I've refactored the monitor code to use a connection interface that's implemented by the architecture-specific builds.

@wardn wardn changed the title Monitor refactoring and devd monitoring for freebsd Refactor monitor and implment devd monitoring for freebsd Feb 12, 2020
wgengine/monitor/monitor.go Outdated Show resolved Hide resolved
wgengine/monitor/monitor.go Show resolved Hide resolved
wgengine/monitor/monitor_freebsd.go Outdated Show resolved Hide resolved
wgengine/monitor/monitor_linux.go Show resolved Hide resolved
wgengine/monitor/monitor_linux.go Outdated Show resolved Hide resolved
wgengine/monitor/monitor_linux.go Outdated Show resolved Hide resolved
Copy link
Member

@danderson danderson left a comment

Choose a reason for hiding this comment

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

Looking good, thank you for generalizing the monitor code and adding platform support! A couple of nits, but after that I think we're good to merge.

func NewConn() (Conn, error) {
conn, err := net.Dial("unixpacket", "/var/run/devd.seqpacket.pipe")
if err != nil {
log.Fatal("Dial error", err)
Copy link
Member

Choose a reason for hiding this comment

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

Return an error, don't log.Fatal from here.

(this is another thing we're bad at in this codebase right now, but libraries shouldn't fatally exit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree... I missed it when porting over some proof of concept code. I've made the change.

}

func NewConn() (Conn, error) {
conn, err := net.Dial("unixpacket", "/var/run/devd.seqpacket.pipe")
Copy link
Member

Choose a reason for hiding this comment

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

Does this unix socket always exist on freebsd? Or is it possible for devd to not be running on a well-behaved system?

I don't know freebsd much, so I'm trying to answer: is it reasonable to assume failure here is a serious "oh crap" error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I didn't have any experience with devd before this implementation, but it seemed like the shortest path to getting it up and running on my box. I'll look into it more, but I figure it's probably better to get some semblance of "working" and iterate from there. I'm happy to drop the _freebsd part of the monitor implementation with this commit and maintain it in my own fork if there's concerns with perceptions on platform support. Let me know.

Copy link
Member

@danderson danderson Feb 13, 2020

Choose a reason for hiding this comment

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

No concerns on platform support, just trying to make sure that the freebsd port is as robust as possible. I don't know how freebsd systems are set up, so I was asking about it :)

man devd says:

The devd utility is a system daemon that runs in the background all the time.

So it sounds to me like that open call should not fail on a normally working system - and so we don't need to make extra effort to recover from this just yet.

Signed-off-by: wardn <wardn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants