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

snet/sciond: add static info metadata fields to snet.PathMetadata #3924

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Oct 29, 2020

Add static info metadata fields, from their temporary location in the path combinator to snet.PathMetadata. Add these fields to the sciond protobuf protocol definitions to transport this information along with sciond path replies.


This change is Reviewable

@matzf matzf force-pushed the snet-path-metadata-staticinfo branch from 4a4cf63 to 37a2050 Compare November 5, 2020 12:55
@matzf matzf marked this pull request as ready for review November 5, 2020 12:56
@matzf matzf force-pushed the snet-path-metadata-staticinfo branch from 37a2050 to dcb5e8e Compare November 5, 2020 13:05
@lukedirtwalker lukedirtwalker self-assigned this Nov 10, 2020
@lukedirtwalker lukedirtwalker self-requested a review November 10, 2020 12:21
Copy link
Collaborator

@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.

Reviewed 10 of 11 files at r1.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @matzf)


go/lib/snet/path.go, line 70 at r1 (raw file):

these entries are signed is signed

I think the is signed needs to be dropped. Also this comment doesn't really hold true for the existing information, for example MTU can't be attributed to each AS, since it's a result of a calculation. I think it would be nice to clarify this in some way.


go/lib/snet/path.go, line 134 at r1 (raw file):

type LinkType uint8

const (

add a comment to this block. // LinkType values


go/lib/snet/path.go, line 141 at r1 (raw file):

)

type GeoCoordinates struct {

Add a comment.

Copy link
Collaborator

@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.

Reviewed 1 of 11 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)

Copy link
Contributor Author

@matzf matzf 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: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)


go/lib/snet/path.go, line 70 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
these entries are signed is signed

I think the is signed needs to be dropped. Also this comment doesn't really hold true for the existing information, for example MTU can't be attributed to each AS, since it's a result of a calculation. I think it would be nice to clarify this in some way.

You're right, not really clear what it was trying to say. Rephrased it, better?


go/lib/snet/path.go, line 134 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

add a comment to this block. // LinkType values

Done.


go/lib/snet/path.go, line 141 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Add a comment.

Done.

Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker
Copy link
Collaborator

/rebase

Add static info metadata fields, from their temporary location in the
path combinator to snet.PathMetadata.
Add to sciond protobuf protocol definitions to transport this
information along with sciond path replies.
@github-actions github-actions bot force-pushed the snet-path-metadata-staticinfo branch from 8b1308b to 79e5644 Compare November 12, 2020 11:46
@lukedirtwalker lukedirtwalker merged commit 3c7fb10 into scionproto:master Nov 12, 2020
@matzf matzf deleted the snet-path-metadata-staticinfo branch November 12, 2020 13:38
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