SNET: remove snet.Addr usage in read/write functions #3583

karampok · 2020-01-07T13:19:24Z

Remove the {write,read}FromSCION function from the snet interface
in order to simplify it. Client of snet/interface should transfer
the net.Addr interface.

Also, it removes appaddr references

This change is

lukedirtwalker

Reviewed 11 of 12 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @karampok)

a discussion (no related file):
The title is not clear to me.

The description should contain that *FromScion functions were removed and the motivation behind this.

go/lib/infra/messenger/messenger.go, line 793 at r1 (raw file):

				ia = v.IA
			case *snet.UDPAddr:
				ia = v.IA

This seems to be a common operation I wonder if it would make sense to have an IAAddr interface in snet:```go
type IAAddr interface {
net.Addr
IA() addr.IA
}

then in cases like this we can just try to cast to it.

Anyway this should probably be in a separate PR.

go/lib/sciond/pathprobe/paths.go, line 147 at r1 (raw file):

addr.HostSVCFromString("NONE")

this could actually just be addr.SvcNone instead.

go/lib/snet/interface.go, line 36 at r1 (raw file):

	Read(b []byte) (int, error)
	ReadFrom(b []byte) (int, net.Addr, error)
	ReadFromSCION(b []byte) (int, *Addr, error)

The package documentation in snet.go still mentions those functions, please update it as well.

go/lib/snet/reader.go, line 46 at r1 (raw file):

ReadFromSCION

ReadFrom

go/lib/snet/reader.go, line 49 at r1 (raw file):

a.ToAddr()

Why is this conversion needed? I mean UDPAddr also implements net.Addr right?

go/lib/snet/reader.go, line 85 at r1 (raw file):

UDP4

unrelated, but could you please fix this, to be UDP

go/lib/snet/reader.go, line 117 at r1 (raw file):

		ip := pkt.Source.Host.IP()
		dup := make(net.IP, len(ip))
		copy(dup, ip)

why not the simple append trick: append(ip[:0:0], ip...)

go/lib/snet/writer.go, line 76 at r2 (raw file):

		}
	case *SVCAddr:
		pkt.SCIONPacketInfo = SCIONPacketInfo{

Apart from host and DstPort this code is exactly the same as above, I think it would be nicer to just extract Host and DstPort in the switch and do everything else below in a common part.

karampok

Reviewable status: 10 of 14 files reviewed, 9 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

The title is not clear to me.

The description should contain that *FromScion functions were removed and the motivation behind this.

I have squashed, does the commit message looks good enough? If not let's iterate together more

go/lib/infra/messenger/messenger.go, line 793 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This seems to be a common operation I wonder if it would make sense to have an IAAddr interface in snet:```go
type IAAddr interface {
net.Addr
IA() addr.IA
}

then in cases like this we can just try to cast to it.

Anyway this should probably be in a separate PR.
</blockquote></details>

I do not  disagree but it is a lot of work, maybe in the future
___
*[go/lib/sciond/pathprobe/paths.go, line 147 at r1](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4ZVBi6RkoEE_F7JXa:-Ly4jseX1H3wcTP2HI2K:b-896fix) ([raw file](https://github.com/scionproto/scion/blob/c4f899be248326fd7fc8299aa487e95d4832f095/go/lib/sciond/pathprobe/paths.go#L147)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

> ```
> addr.HostSVCFromString("NONE")
> ```
this could actually just be `addr.SvcNone` instead.
</blockquote></details>

Done.
___
*[go/lib/snet/interface.go, line 36 at r1](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4VAPx1RVA0WGMhuwi:-Ly4k5J01X_bwpoh9b-Z:b-896fix) ([raw file](https://github.com/scionproto/scion/blob/7566bfb825541d5ae85c5db1d679819a95bdf936/go/lib/snet/interface.go#L36)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

The package documentation in `snet.go` still mentions those functions, please update it as well.
</blockquote></details>

Done.
___
*[go/lib/snet/reader.go, line 46 at r1](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4Utwk5B1vfDsv6Lnz:-Ly4k7Ma1nkUkN8nqsT-:b-20ufd5) ([raw file](https://github.com/scionproto/scion/blob/c4f899be248326fd7fc8299aa487e95d4832f095/go/lib/snet/reader.go#L46)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

> ```
> ReadFromSCION
> ```
`ReadFrom`
</blockquote></details>

Done. 
I am not sure though the `already known` is anymore a thing.
___
*[go/lib/snet/reader.go, line 49 at r1](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4W-n22CS8qiTdsYSo:-Ly4eLB54hTE9h4AoUwR:b-cr9s20) ([raw file](https://github.com/scionproto/scion/blob/c4f899be248326fd7fc8299aa487e95d4832f095/go/lib/snet/reader.go#L49)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

> ```
> a.ToAddr()
> ```
Why is this conversion needed? I mean UDPAddr also implements `net.Addr` right?
</blockquote></details>

There are static assumptiosn that the net.Addr is of type snet.Addr add over.
https://github.com/Anapaya/scion/issues/1773 should be done, and then we remove the `ToAddr()`

I suggest that as intermediate step, but I could also wait until we solve the above issue
___
*[go/lib/snet/reader.go, line 85 at r1](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4VOuoCbfNs5KtWzRw:-Ly4fL_ZBUFcGyRg0n_U:b-896fix) ([raw file](https://github.com/scionproto/scion/blob/c4f899be248326fd7fc8299aa487e95d4832f095/go/lib/snet/reader.go#L85)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

> ```
> UDP4
> ```
unrelated, but could you please fix this, to be `UDP`
</blockquote></details>

Done.
___
*[go/lib/snet/reader.go, line 117 at r1](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4VeP6EqY1VXtd_jzm:-Ly4fYQv4PvhU8PvoR_-:b-311uk8) ([raw file](https://github.com/scionproto/scion/blob/c4f899be248326fd7fc8299aa487e95d4832f095/go/lib/snet/reader.go#L117)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

why not the simple append trick: `append(ip[:0:0], ip...)`
</blockquote></details>

and I was looking for the most one-line hacky way to do that, thx!
___
*[go/lib/snet/writer.go, line 76 at r2](https://reviewable.io/reviews/scionproto/scion/3583#-Ly4c2dMBGegPSNUU2oE:-Ly4ify44RKUk_4u2IoM:b-7rl0jf) ([raw file](https://github.com/scionproto/scion/blob/8a4e40da1717de5736f9291da645b97b8a982acd/go/lib/snet/writer.go#L76)):*
<details><summary><i>Previously, lukedirtwalker (Lukas Vogel) wrote…</i></summary><blockquote>

Apart from host and DstPort this code is exactly the same as above, I think it would be nicer to just extract Host and DstPort in the switch and do everything else below in a common part.
</blockquote></details>

done,


<!-- Sent from Reviewable.io -->

lukedirtwalker

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karampok)

a discussion (no related file):

Previously, karampok (Konstantinos) wrote…

I have squashed, does the commit message looks good enough? If not let's iterate together more

looks better I would probably do s/Addr/snet.Addr/ for more clarity. Please also update the PR title and description to match the commit.

go/lib/snet/reader.go, line 46 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Done.
I am not sure though the already known is anymore a thing.

Neither am I, you'd have to ask @scrye

go/lib/snet/snet.go, line 29 at r3 (raw file):

// ... WriteTo
// and WriteTo ...

once is enough ;)

karampok

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

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

looks better I would probably do s/Addr/snet.Addr/ for more clarity. Please also update the PR title and description to match the commit.

Done.

go/lib/snet/reader.go, line 46 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Neither am I, you'd have to ask @scrye

I remove, because i cannot see it anymore in the code such behavior.

go/lib/snet/snet.go, line 29 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

// ... WriteTo
// and WriteTo ...
once is enough ;)

Done.

Remove the {write,read}FromSCION function from the snet interface in order to simplify it. Client of snet/interface should transfer the net.Addr interface. Also, it removes appaddr references

lukedirtwalker

Reviewed 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

karampok force-pushed the pub-issue-1599 branch 9 times, most recently from d2524fa to c957e0f Compare January 8, 2020 12:56

karampok marked this pull request as ready for review January 8, 2020 13:33

karampok requested a review from lukedirtwalker January 8, 2020 13:33

karampok changed the title ~~Remove writeTOSCION from snet - wip~~ Remove Addr usage from snet read/write Jan 8, 2020

lukedirtwalker requested changes Jan 8, 2020

View reviewed changes

karampok force-pushed the pub-issue-1599 branch from 5f0ab3e to 872cc6d Compare January 8, 2020 15:43

karampok commented Jan 8, 2020

View reviewed changes

lukedirtwalker requested changes Jan 8, 2020

View reviewed changes

karampok force-pushed the pub-issue-1599 branch from 872cc6d to 94cd832 Compare January 9, 2020 09:08

karampok changed the title ~~Remove Addr usage from snet read/write~~ SNET: remove snet.Addr usage in read/write functions Jan 9, 2020

karampok commented Jan 9, 2020

View reviewed changes

SNET: do not use snet.Addr in read/write functions

acc1eea

Remove the {write,read}FromSCION function from the snet interface in order to simplify it. Client of snet/interface should transfer the net.Addr interface. Also, it removes appaddr references

karampok force-pushed the pub-issue-1599 branch from 94cd832 to acc1eea Compare January 9, 2020 09:26

lukedirtwalker approved these changes Jan 9, 2020

View reviewed changes

karampok merged commit efc707e into scionproto:master Jan 9, 2020

karampok deleted the pub-issue-1599 branch January 9, 2020 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SNET: remove snet.Addr usage in read/write functions #3583

SNET: remove snet.Addr usage in read/write functions #3583

Uh oh!

karampok commented Jan 7, 2020 •

edited

Loading

Uh oh!

lukedirtwalker left a comment

Uh oh!

karampok left a comment

Uh oh!

lukedirtwalker left a comment

Uh oh!

karampok left a comment

Uh oh!

lukedirtwalker left a comment

Uh oh!

Uh oh!

SNET: remove snet.Addr usage in read/write functions #3583

SNET: remove snet.Addr usage in read/write functions #3583

Uh oh!

Conversation

karampok commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

lukedirtwalker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karampok commented Jan 7, 2020 •

edited

Loading