Path test in showpaths tool #2108

sustrik · 2018-11-08T13:11:20Z

Fixes #2039

This change is

sgmonroy

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @sustrik and @sgmonroy)

go/tools/showpaths/paths.go, line 115 at r1 (raw file):

	}
	for i := len(pathStatuses); i > 0; i-- {
		b := make([]byte, 65536, 65536)

Why such a big buffer? errors should fit in 1500 Bytes packets and if it doesn't fit you should get an error from ReadFrom

go/tools/showpaths/paths.go, line 116 at r1 (raw file):

	for i := len(pathStatuses); i > 0; i-- {
		b := make([]byte, 65536, 65536)
		_, addr, err := scionConn.ReadFromSCION(b)

you should have a timeout here in case some packets are lost and you get no reply at all

go/tools/showpaths/paths.go, line 117 at r1 (raw file):

		b := make([]byte, 65536, 65536)
		_, addr, err := scionConn.ReadFromSCION(b)
		if _, ok := err.(*snet.OpError); !ok {

I don't think this is enough. You should check the SCMP error you got back, and check for the expected class/type error.
You can call SCMP() on an OpError then check for C_Rourintg/T_R_BadHost

As discussed offline, we should check for:

expected SCMP error C_Rourintg/T_R_BadHost
print any other error
unexpected packet

sgmonroy

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

sustrik · 2018-11-08T14:48:03Z

go/tools/showpaths/paths.go, line 116 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

you should have a timeout here in case some packets are lost and you get no reply at all

There's a timeout set on the connection, see line 84

sustrik · 2018-11-08T14:48:42Z

go/tools/showpaths/paths.go, line 115 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

Why such a big buffer? errors should fit in 1500 Bytes packets and if it doesn't fit you should get an error from ReadFrom

I wasn't sure what the limit is. Shrinking it to 1500.

sustrik · 2018-11-08T14:48:57Z

go/tools/showpaths/paths.go, line 115 at r1 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

I wasn't sure what the limit is. Shrinking it to 1500.

Done.

sustrik · 2018-11-08T14:49:08Z

go/tools/showpaths/paths.go, line 116 at r1 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

There's a timeout set on the connection, see line 84

Done.

sustrik · 2018-11-09T07:48:34Z

go/tools/showpaths/paths.go, line 117 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

I don't think this is enough. You should check the SCMP error you got back, and check for the expected class/type error.
You can call SCMP() on an OpError then check for C_Rourintg/T_R_BadHost

As discussed offline, we should check for:

expected SCMP error C_Rourintg/T_R_BadHost

print any other error

unexpected packet

Done.

sgmonroy

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @sgmonroy and @sustrik)

go/tools/showpaths/paths.go, line 77 at r2 (raw file):

	pathStatuses := make(map[string]string)
	if *status {

the code for this functionality is quite large compare to the main body itself, could you take it out of main and brake it up a bit into smaller function/functions?

sgmonroy

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sustrik)

sustrik · 2018-11-09T08:48:02Z

go/tools/showpaths/paths.go, line 77 at r2 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

the code for this functionality is quite large compare to the main body itself, could you take it out of main and brake it up a bit into smaller function/functions?

I've split the path testing into a separate function. However, splitting it further doesn't make much sense. Sending the packets alone isn't good for anything. Neither is receiving the replies.

sgmonroy

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @scrye)

sgmonroy

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @scrye)

sgmonroy · 2018-11-09T13:18:24Z

a discussion (no related file):
Let Sergiu have a look before merging

scrye

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @sustrik)

go/tools/showpaths/paths.go, line 81 at r3 (raw file):

	}
	i := 0
	for _, path := range reply.Entries {

It's not related to this PR specifically, but we can simplify two lines by doing:

for i, path := range reply.Entries {
  // ....
}

go/tools/showpaths/paths.go, line 91 at r3 (raw file):

		}
		fmt.Printf("\n")
		i++

This must be removed for the previous comment to work.

go/tools/showpaths/paths.go, line 148 at r3 (raw file):

	pathStatuses := make(map[string]string)
	for _, path := range paths {
		sPath := spath.New(path.Path.FwdPath)

It would be a bit easier to read this if the inside of the loop were split into a different function:

for _, path := range paths {
    sendTestPacket(scionConn, path)
    pathStatuses[string(path.Path.FwdPath)] = "Timeout"
}

go/tools/showpaths/paths.go, line 173 at r3 (raw file):

	}
	for i := len(pathStatuses); i > 0; i-- {
		b := make([]byte, 1500, 1500)

Same suggestion as above:

for i := len(pathStatuses); i > 0; i-- {
    path, status := receiveNextTestReply(scionConn)
    pathStatuses[path] = status // and other global logic, like double replies
}

scrye

Dismissed @sgmonroy from a discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sustrik)

sustrik · 2018-11-09T14:37:39Z

go/tools/showpaths/paths.go, line 81 at r3 (raw file):

Previously, scrye (Sergiu Costea) wrote…

It's not related to this PR specifically, but we can simplify two lines by doing:
for i, path := range reply.Entries {
  // ....
}

Done.

sustrik · 2018-11-09T14:37:45Z

go/tools/showpaths/paths.go, line 91 at r3 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This must be removed for the previous comment to work.

Done.

sustrik · 2018-11-09T14:37:50Z

go/tools/showpaths/paths.go, line 148 at r3 (raw file):

Previously, scrye (Sergiu Costea) wrote…

It would be a bit easier to read this if the inside of the loop were split into a different function:
for _, path := range paths {
    sendTestPacket(scionConn, path)
    pathStatuses[string(path.Path.FwdPath)] = "Timeout"
}

Done.

sustrik · 2018-11-09T14:37:55Z

go/tools/showpaths/paths.go, line 173 at r3 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Same suggestion as above:

for i := len(pathStatuses); i > 0; i-- {
    path, status := receiveNextTestReply(scionConn)
    pathStatuses[path] = status // and other global logic, like double replies
}

Done.

scrye

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

go/tools/showpaths/paths.go, line 96 at r4 (raw file):

	var err error

	flag.Parse()

I just did some manual testing and realized logging is not correctly initialized. The following needs to be added after this line:

log.SetupFromFlags("")

go/tools/showpaths/paths.go, line 182 at r4 (raw file):

		Path:    sPath,
	}
	fmt.Println("Sending test packet to: ", addr)

This isn't very informative to the user, because it only prints something like the below on start:

Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)

It can probably be dropped altogether, or changed to log.Debug and having it also log the path:

log.Debug("Sending test packet on: ", "path", addr.Path)

go/tools/showpaths/paths.go, line 210 at r4 (raw file):

	}
	if opErr, ok := err.(*snet.OpError); ok {
		fmt.Println(opErr)

This also can probably be dropped/changed to debug/info logging, because it is very advanced information for users:

Class=ROUTING(1) Type=BAD_HOST(6) TotalLen=64B Checksum=1fff Timestamp=2018-11-09 16:13:23.624934 +0100 CET

If it might be important information, it should be translated to something more human readable.

sustrik

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @scrye)

go/tools/showpaths/paths.go, line 96 at r4 (raw file):

Previously, scrye (Sergiu Costea) wrote…

I just did some manual testing and realized logging is not correctly initialized. The following needs to be added after this line:
log.SetupFromFlags("")

Done.

go/tools/showpaths/paths.go, line 182 at r4 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This isn't very informative to the user, because it only prints something like the below on start:
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
Sending test packet to:  1-ff00:0:111,[UNKNOWN M (0xffff)]:0 (UDP)
It can probably be dropped altogether, or changed to log.Debug and having it also log the path:
log.Debug("Sending test packet on: ", "path", addr.Path)

Done. Also, let's print an entire path, just in case.

go/tools/showpaths/paths.go, line 210 at r4 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This also can probably be dropped/changed to debug/info logging, because it is very advanced information for users:
Class=ROUTING(1) Type=BAD_HOST(6) TotalLen=64B Checksum=1fff Timestamp=2018-11-09 16:13:23.624934 +0100 CET
If it might be important information, it should be translated to something more human readable.

This was just a debugging message I forgot to remove. The human-readable error message is printed alongside the path in the results.

scrye

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

Fixes #2039

sustrik requested a review from sgmonroy November 8, 2018 13:12

sgmonroy suggested changes Nov 8, 2018

View reviewed changes

sgmonroy reviewed Nov 8, 2018

View reviewed changes

sgmonroy suggested changes Nov 9, 2018

View reviewed changes

sgmonroy reviewed Nov 9, 2018

View reviewed changes

sgmonroy requested a review from scrye November 9, 2018 13:16

sgmonroy approved these changes Nov 9, 2018

View reviewed changes

scrye suggested changes Nov 9, 2018

View reviewed changes

scrye reviewed Nov 9, 2018

View reviewed changes

scrye suggested changes Nov 9, 2018

View reviewed changes

sustrik commented Nov 9, 2018

View reviewed changes

scrye approved these changes Nov 9, 2018

View reviewed changes

Path test in showpaths tool

dff8442

Fixes #2039

sustrik merged commit 3132e6b into scionproto:master Nov 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path test in showpaths tool #2108

Path test in showpaths tool #2108

sustrik commented Nov 8, 2018 •

edited by kormat

Loading

sgmonroy left a comment

sgmonroy left a comment

sustrik commented Nov 8, 2018

sustrik commented Nov 8, 2018

sustrik commented Nov 8, 2018

sustrik commented Nov 8, 2018

sustrik commented Nov 9, 2018

sgmonroy left a comment

sgmonroy left a comment

sustrik commented Nov 9, 2018

sgmonroy left a comment

sgmonroy left a comment

sgmonroy commented Nov 9, 2018

scrye left a comment

scrye left a comment

sustrik commented Nov 9, 2018

sustrik commented Nov 9, 2018

sustrik commented Nov 9, 2018

sustrik commented Nov 9, 2018

scrye left a comment

sustrik left a comment

scrye left a comment

Path test in showpaths tool #2108

Path test in showpaths tool #2108

Conversation

sustrik commented Nov 8, 2018 • edited by kormat Loading

sgmonroy left a comment

Choose a reason for hiding this comment

sgmonroy left a comment

Choose a reason for hiding this comment

sustrik commented Nov 8, 2018

sustrik commented Nov 8, 2018

sustrik commented Nov 8, 2018

sustrik commented Nov 8, 2018

sustrik commented Nov 9, 2018

sgmonroy left a comment

Choose a reason for hiding this comment

sgmonroy left a comment

Choose a reason for hiding this comment

sustrik commented Nov 9, 2018

sgmonroy left a comment

Choose a reason for hiding this comment

sgmonroy left a comment

Choose a reason for hiding this comment

sgmonroy commented Nov 9, 2018

scrye left a comment

Choose a reason for hiding this comment

scrye left a comment

Choose a reason for hiding this comment

sustrik commented Nov 9, 2018

sustrik commented Nov 9, 2018

sustrik commented Nov 9, 2018

sustrik commented Nov 9, 2018

scrye left a comment

Choose a reason for hiding this comment

sustrik left a comment

Choose a reason for hiding this comment

scrye left a comment

Choose a reason for hiding this comment

sustrik commented Nov 8, 2018 •

edited by kormat

Loading