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

Fix sig/mgmt/Addr #3386

Merged
merged 1 commit into from
Nov 15, 2019
Merged

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Nov 15, 2019

We can't use UDPAddr on the wire format, instead use hostinfo again.

Fixes tests broken by #3370


This change is Reviewable

We can't use UDPAddr on the wire format, instead use hostinfo again.

Fixes tests broken by  scionproto#3370
Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion


go/sig/mgmt/addr.go, line 30 at r1 (raw file):

type Addr struct {
	Ctrl      *net.UDPAddr

alternatively we could introduce a wire type for net.UDPAddr?
The problem is the IP field in net.UDPAddr it maps to iP in capnp.

@scrye scrye self-requested a review November 15, 2019 12:38
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)


go/sig/mgmt/addr.go, line 30 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

alternatively we could introduce a wire type for net.UDPAddr?
The problem is the IP field in net.UDPAddr it maps to iP in capnp.

Or we just move around a string... "192.168.0.0:60000" is a clean way of writing an address which is not subject to library specific encodings (e.g., Go sometimes encodes IPv4 addresses in 16 bytes, see net.IPv4, which causes havoc when talking to other processes)... And there are ways to encode both IPv4 and IPv6 via strings.

@lukedirtwalker lukedirtwalker merged commit 9a26911 into scionproto:master Nov 15, 2019
@lukedirtwalker lukedirtwalker deleted the pubFixSIG branch November 15, 2019 12:55
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.

2 participants