Skip to content

Conversation

@gfr10598
Copy link
Contributor

@gfr10598 gfr10598 commented May 8, 2018

  1. Set up travis
  2. Add badges to README.md
  3. Import inetdiag utility code and adapt for our use.
  4. Add some unit tests and get coverage to reasonable level.

Part of story #2


This change is Reviewable

@gfr10598 gfr10598 requested a review from pboothe May 8, 2018 15:04
@gfr10598
Copy link
Contributor Author

gfr10598 commented May 8, 2018

Issue #2

@pboothe
Copy link

pboothe commented May 8, 2018

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.


.travis.yml, line 64 at r1 (raw file):

# the non-integration tests are repeated.  If we change the unit tests to NOT run when integration
# test tag is set, then we would need to have separate cov files.
# Note: we do not run integration tests from forked PRs b/c the SA is unavailable.

"from forked repos" not "from forked PRs"


inetdiag/inetdiag.go, line 38 at r1 (raw file):

// netinet/tcp.h
const (
	_               = iota

Won't this cause lint warnings because local variable _ is shadowing global variable _? Or is go smart enough to realize that _ is a trash variable name?


inetdiag/inetdiag.go, line 56 at r1 (raw file):

)

var tcpStatesMap = map[uint8]string{

https://godoc.org/golang.org/x/tools/cmd/stringer ?


inetdiag/inetdiag.go, line 96 at r1 (raw file):

}

// TODO should use more net.IP code instead of custom code.

Make issue, link to issue :)


inetdiag/inetdiag.go, line 105 at r1 (raw file):

}

func isIpv6(original [16]byte) bool {

Let's just hope we never get a very-improbable v6 address like AB:CD:EF:12:00:00:00:00:00:00:00:00:00:00:00:00 ?


inetdiag/inetdiag.go, line 126 at r1 (raw file):

}

// InetDiagReqV2 is the Netlink request struct.

as defined in...


inetdiag/inetdiag.go, line 161 at r1 (raw file):

// InetDiagMsg is the linux binary representation of a InetDiag message header.  Note that this
// does NOT take into account byte ordering.

I don't understand whether this means I should correct the byte ordering before I put things in to this struct or after I get them out or just ignore the issue altogether.


inetdiag/inetdiag_test.go, line 122 at r1 (raw file):

	hdr, _ := inetdiag.ParseInetDiagMsg(data[:])

	if hdr.ID.SrcIP().IsLoopback() {

It seems like there are more conditions being tested here than just "is it loopback" ?


Comments from Reviewable

@gfr10598
Copy link
Contributor Author

gfr10598 commented May 8, 2018

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions.


.travis.yml, line 64 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

"from forked repos" not "from forked PRs"

Wow - you actually read this? Copied from other travis files, so I don't even see the old comments. Thanks.


inetdiag/inetdiag.go, line 38 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Won't this cause lint warnings because local variable _ is shadowing global variable _? Or is go smart enough to realize that _ is a trash variable name?

_ is go syntax for an unused value. I don't think you can actually reference it.


inetdiag/inetdiag.go, line 56 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

https://godoc.org/golang.org/x/tools/cmd/stringer ?

Interesting tool. Will keep it in mind.


inetdiag/inetdiag.go, line 96 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Make issue, link to issue :)

I don't think it merits its own issue. I would rate it p4 and we would never look at it.


inetdiag/inetdiag.go, line 105 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Let's just hope we never get a very-improbable v6 address like AB:CD:EF:12:00:00:00:00:00:00:00:00:00:00:00:00 ?

This is consistent with how the net.IP library works, so I'm not too worried.
8-)


inetdiag/inetdiag.go, line 126 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

as defined in...

Done.


inetdiag/inetdiag.go, line 161 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

I don't understand whether this means I should correct the byte ordering before I put things in to this struct or after I get them out or just ignore the issue altogether.

Oops. I added that earlier and discovered it is misleading. Fixed.


inetdiag/inetdiag_test.go, line 122 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

It seems like there are more conditions being tested here than just "is it loopback" ?

Maybe you were looking at an older version? Not sure how that would have happened, but I did have some other stuff in earlier commits.


Comments from Reviewable

@gfr10598
Copy link
Contributor Author

gfr10598 commented May 8, 2018

PTAL.


Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@pboothe
Copy link

pboothe commented May 8, 2018

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


.travis.yml, line 64 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Wow - you actually read this? Copied from other travis files, so I don't even see the old comments. Thanks.

But the text is unchanged?


inetdiag/inetdiag.go, line 96 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I don't think it merits its own issue. I would rate it p4 and we would never look at it.

But if you make it an issue, then we can ask someone else to fix it. :)


inetdiag/inetdiag_test.go, line 122 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Maybe you were looking at an older version? Not sure how that would have happened, but I did have some other stuff in earlier commits.

I was unclear and too brief.

The function TestID6 does a whole lot of arranging bytes just so. However, the only assertions that the test makes (as near as I can tell) are "Do hdr.ID.SrcIP().IsLoopback() and hdr.ID.DstIP().IsLoopback() both return false?"

The amount of boilerplate and setup seems out of place compared to the rigorousness of the tests. Are more than those two conditions being tested? If so, where? If not, should they be?


Comments from Reviewable

@gfr10598
Copy link
Contributor Author

gfr10598 commented May 8, 2018

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


.travis.yml, line 64 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

But the text is unchanged?

Oops. Got distracted.


inetdiag/inetdiag.go, line 96 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

But if you make it an issue, then we can ask someone else to fix it. :)

grep TODO... 8-)


inetdiag/inetdiag_test.go, line 122 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

I was unclear and too brief.

The function TestID6 does a whole lot of arranging bytes just so. However, the only assertions that the test makes (as near as I can tell) are "Do hdr.ID.SrcIP().IsLoopback() and hdr.ID.DstIP().IsLoopback() both return false?"

The amount of boilerplate and setup seems out of place compared to the rigorousness of the tests. Are more than those two conditions being tested? If so, where? If not, should they be?

Got it. Cleaned it up. Thanks.


Comments from Reviewable

@pboothe
Copy link

pboothe commented May 8, 2018

:lgtm:


Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@gfr10598 gfr10598 merged commit 9a29bbc into master May 8, 2018
@gfr10598 gfr10598 deleted the inetdiag branch May 8, 2018 21:22
gfr10598 added a commit that referenced this pull request May 9, 2018
Basic repo setup, and import inetdiag utilities.
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.

3 participants