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

Go dispatcher: Add SCMP ID tracking tables #2326

Merged
merged 2 commits into from
Jan 11, 2019
Merged

Go dispatcher: Add SCMP ID tracking tables #2326

merged 2 commits into from
Jan 11, 2019

Conversation

scrye
Copy link
Contributor

@scrye scrye commented Jan 11, 2019

This is a stepping stone to implementing SCMP Echo, TraceRoute, and RecordPath support in the Go dispatcher.


This change is Reviewable

@scrye scrye added the c/dispatcher SCION dispatcher label Jan 11, 2019
@scrye scrye self-assigned this Jan 11, 2019
@scrye scrye requested a review from lukedirtwalker January 11, 2019 15:06
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 5 of 5 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scrye)


go/godispatcher/internal/registration/iatable_test.go, line 149 at r1 (raw file):

		err = ref.RegisterID(42)
		xtest.FailOnErr(t, err)
		Convey("Performing SCMP lookup now succeeds", func() {

Nit: the now doesn't really make sense (only with the Test above, but that is in a different function)


go/godispatcher/internal/registration/scmp_table.go, line 34 at r1 (raw file):

func (t *SCMPTable) Lookup(id uint64) (interface{}, bool) {
	value, ok := t.m[id]
	return value, ok

This could be collapsed into return t.m[id]


go/godispatcher/internal/registration/scmp_table.go, line 50 at r1 (raw file):

func (t *SCMPTable) Remove(id uint64) error {
	if _, ok := t.m[id]; !ok {
		return common.NewBasicError("element not found", nil, "id", id)

Why is the error needed? Default go delete also doesn't complain if an entry is deleted that isn't there?

Copy link
Contributor Author

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

Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)


go/godispatcher/internal/registration/scmp_table.go, line 34 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This could be collapsed into return t.m[id]

Sadly, it can't. The compiler assumes the one return value variant of map access if used in the return, so it complains that it's returning only one value instead of two.


go/godispatcher/internal/registration/scmp_table.go, line 50 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Why is the error needed? Default go delete also doesn't complain if an entry is deleted that isn't there?

I was thinking it's useful to catch double frees, but it leads to some awkward untestable branches. I dropped the return value, it's more elegant.

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.

:lgtm:

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


go/godispatcher/internal/registration/scmp_table.go, line 34 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Sadly, it can't. The compiler assumes the one return value variant of map access if used in the return, so it complains that it's returning only one value instead of two.

wow TIL

@scrye scrye merged commit e892ebe into scionproto:master Jan 11, 2019
@scrye scrye deleted the pubpr-disp-id-interceptor branch January 11, 2019 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/dispatcher SCION dispatcher
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants