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 connected Write on snet.Conn #3617

Merged
merged 2 commits into from
Jan 20, 2020
Merged

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Jan 20, 2020

Minimal fix for snet.Conn.Write.
Previously, passing nil into WriteTo would automatically pick the
connected remote address, if available. With some of the previous
refactoring (amongst other, in #3571), this is no longer the case. Right
now, Write is simply broken.
Apply the simplest fix, just specify to use the connected address
(base.remote) in Write. Note that the behaviour is still subtly
different than it used to be (e.g. no error if WriteTo is invoked on a
connected socket), but this seems to be the accepted behaviour change
within the refactoring in #3571.

Additionally, add a specific error message for the nil address case
in WriteTo, as "Unable to write to non-SCION address" was misleading.


This change is Reviewable

Minimal fix for `snet.Conn.Write`.
Previously, passing `nil` into `WriteTo` would automatically pick the
connected remote address, if available. With some of the previous
refactoring (amongst other, in scionproto#3571), this is no longer the case. Right
now, `Write` is simply broken.
Apply the simplest fix, just specify to use the connected address
(`base.remote`) in `Write`. Note that the behaviour is still subtly
different than it used to be (e.g. no error if WriteTo is invoked on a
connected socket), but this seems to be the accepted behaviour change
within the refactoring in scionproto#3571.

Additionally, add a specific error message for the `nil` address case
in `WriteTo`, as "Unable to write to non-SCION address" was misleading.
@matzf matzf requested a review from karampok January 20, 2020 08:37
Copy link
Contributor

@karampok karampok 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 1 files reviewed, 1 unresolved discussion (waiting on @karampok and @matzf)


go/lib/snet/writer.go, line 74 at r1 (raw file):

		if a == nil {
			return 0, common.NewBasicError("Missing remote address", nil)
		} else {

Ideally we would like to have an implementation without else
Consider something like this

switch x.(type) {
	case nil:
		fmt.Println("x is nil") // here v has type interface{}
	case *myType:
		fmt.Println("x is myType") 
	default:
		fmt.Println("type unknown") // here v has type interface{}
	}

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: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @karampok)


go/lib/snet/writer.go, line 74 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Ideally we would like to have an implementation without else
Consider something like this

switch x.(type) {
	case nil:
		fmt.Println("x is nil") // here v has type interface{}
	case *myType:
		fmt.Println("x is myType") 
	default:
		fmt.Println("type unknown") // here v has type interface{}
	}

Good point, thanks. Done.

Copy link
Contributor

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

@karampok karampok merged commit 02894ff into scionproto:master Jan 20, 2020
@matzf matzf deleted the fix-snet-write branch January 20, 2020 15:15
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