Refactor main networking code #8

reethikar · 2022-08-05T21:19:14Z

Refactor code to improve readability and modularity
For now,

I've pushed zeroTrace items to its own file, and refactored the functions
Reduced the number of variables functions take, and use a zeroTrace instance to carry necessary variables

This is still a work in progress

Refactor code to improve readability and modular

…d as parameter

… and filter in one func

reethikar · 2022-08-08T13:45:17Z

@NullHypothesis marking this ready to get preliminary feedback on how I'm going about with the refactoring. Let me know if this is a good start.

…riables

NullHypothesis · 2022-08-08T21:01:17Z

Unrelated to the actual code: Take a look at the commit guidelines of the Pro Git book. You're doing pretty well on that front already but the section contains a few helpful suggestions, like limiting the first line of a commit message to <=50 characters.

NullHypothesis

Left a bunch of comments. I hope it's helpful!

NullHypothesis · 2022-08-08T21:15:58Z

cmd/test-server/handlers.go

+	clientIP, clPort, _ := net.SplitHostPort(clientIPstr)
+	clientPort, _ := strconv.Atoi(clPort)
+
+	zeroTraceInstance := newZeroTrace(deviceName, myConn, uuid, clientIP, clientPort)


I would only pass myConn to newZeroTrace. The function can then extract IP addresses and ports from the connection. No need to pass them separately.

Fixed in 7dd7051

That's a big improvement in clarity. Nice!

NullHypothesis · 2022-08-08T21:23:24Z

cmd/test-server/util.go

@@ -16,3 +21,61 @@ func isValidUUID(u string) bool {
 func fmtTimeMs(value time.Duration) float64 {
 	return (float64(value) / float64(time.Millisecond))
 }
+
+// getMeanIcmpRTT gets Avg RTT from all successful ICMP measurements, to display on webpage
+func getMeanIcmpRTT(icmp []RtItem) float64 {


What is RtItem short for? Round Trip Item? I've always been a bit confused by the name. Some suggestions for a better name:

RttResult

RttMsmt

HopResult

HopMsmt

Fixed in 7dd7051

NullHypothesis · 2022-08-08T21:28:59Z

cmd/test-server/zerotrace.go

+
+const (
+	beginTTLValue        = 5
+	MaxTTLHops           = 32


Inconsistent capitalization of constants. I generally suggest lowercase unless you want to export a constant for use outside of this package.

Fixed in 7dd7051

NullHypothesis · 2022-08-08T21:30:23Z

cmd/test-server/zerotrace.go

+)
+
+var (
+	buffer  gopacket.SerializeBuffer


Some of these variables are only used once and don't need to be global. In that case, I suggest inlining them.

Fixed in 7dd7051 and 0e14997

NullHypothesis · 2022-08-08T21:34:15Z

cmd/test-server/zerotrace.go

+	icmpPktError = errors.New("IP header unavailable")
+)
+
+type SentPacketData struct {


Maybe add a doc string explaining what this is used for? It's not immediately clear from the name only.

Added in 0e14997

NullHypothesis · 2022-08-08T21:46:02Z

cmd/test-server/zerotrace.go

+}
+
+// recvPackets listens on the provided pcap handler for packets sent, processes TCP and ICMP packets differently
+func (z *zeroTrace) recvPackets(pcapHdl *pcap.Handle, hops chan HopRTT) {


Is it possible that this function won't return if we never get the ICMP packets that we're expecting? It should probably take as input some sort of abort signal from the caller, instructing the function to return when the caller is done.

That's right. I'll add an abort signal that makes this function return when the tracerouteHopTimeout is reached (and yet no ICMP packets are received) for the last probe. Added in c08b5db.

NullHypothesis · 2022-08-08T21:56:09Z

cmd/test-server/zerotrace.go

+		return nil, err
+	}
+	if err = pcapHdl.SetBPFFilter(fmt.Sprintf("(tcp and port %d and host %s) or icmp", z.ClientPort, z.ClientIP)); err != nil {
+		ErrLogger.Fatal(err)


Let's not exit the program here, and simply propagate the error. It should be up to the code that's using zeroTrace to decide what to do with that error.

Fixed in b5c2e3d

NullHypothesis · 2022-08-09T00:11:04Z

cmd/test-server/zerotrace.go

+	// Create the packet with the layers
+	buffer = gopacket.NewSerializeBuffer()
+
+	serializeErr := gopacket.SerializeLayers(buffer, options,


Nitpick: I'd simply call each error err. That's both idiomatic and reduces the cognitive load of reviewers because if every error is called err, there are fewer variables to keep track of.

Done in ffe573f

NullHypothesis · 2022-08-09T00:15:12Z

cmd/test-server/zerotrace.go

+		gopacket.Payload(rawBytes),
+	)
+	if serializeErr != nil {
+		ErrLogger.Println("Send Packet Error: Serialize: ", serializeErr)


We could save ourselves all these log messages if the caller checks if sendTracePacket returned and error, and if so, we can do something like:

if err := z.sendTracePacket(ttlValue); err != nil { ErrLogger.Println("Send Packet Error: ", err) return traceroute, sendError }

Agreed, and done in ffe573f

NullHypothesis · 2022-08-09T00:17:16Z

cmd/test-server/zerotrace.go

+	PcapHdl          *pcap.Handle
+	ClientIP         string
+	ClientPort       int
+	IPIdHop          map[int][]SentPacketData


Are we anticipating concurrent reads/writes to this map?

(I've renamed this map to SentPktsIPId in 0e14997)
It is a map of the IPIds and sent times of the sent packets for each TTL. I don't expect concurrent reads or writes because each packet---sent packet (writes to this) or received packet (reads from this)---is dealt with sequentially.

Fix inconsistent capitalization, inline variables, don't pass variables that can be derived

Remove excessive logging and log at caller, avoid special err variables

Add a html form to have users submit contact info to help debug

reethikar · 2022-08-16T15:54:15Z

cmd/test-server/zerotrace.go

+		select {
+		case <-quit:
+			return


I've found an issue with this:
In this file L:113, we feed "true" to the quit channel once the sendTTLIncrementingProbes function returns. But since I am working with the packet stream here, I need to wait until another packet is seen on this handle before being able to quit. Moving the select statement out also does not work since the execution will be stuck in the for packet := range packetStream.Packets() loop.

Trying to fix this now

A fix has been pushed in a89c1d2

That's the same fix that I had in mind when reviewing the code 👍

Fix issue by reading from packet stream as a channel, do not check for isClientReached? in recvPackets, instead let sender figure it out and abort the receive goroutine

NullHypothesis · 2022-08-17T02:05:36Z

Thanks for addressing all the feedback and for doing it in separate commits. That makes it much easier for me to see what changed. Let's not have the perfect be the enemy of the good, and merge this. We can still improve the code and add tests in follow-up PRs.

Input form to collect user details

reethikar · 2022-08-17T13:55:11Z

Merged PR#9 into this and then this PR into main. Thanks for your comments! 🚀

reethikar added 3 commits August 5, 2022 17:17

Refaactor main networking code

b6dce6d

Refactor code to improve readability and modular

Return half-done traceroute results, and remove passing redundant uui…

8c0a410

…d as parameter

Add client IP and client Port to zeroTrace struct, combine pcap setup…

36f07dd

… and filter in one func

reethikar changed the title ~~Refaactor main networking code~~ Refactor main networking code Aug 8, 2022

reethikar marked this pull request as ready for review August 8, 2022 13:44

reethikar requested a review from NullHypothesis August 8, 2022 13:44

reethikar added 4 commits August 8, 2022 09:59

Gofmt, cleanup variables and simplify variable use

f07d122

Move process ICMP and TCP packets under zerotrace to reduce global va…

8b44aa8

…riables

Fix imports

170e0aa

Simplify extractTracerouteHopData func

165e352

reethikar added 2 commits August 8, 2022 18:27

Remove redundant icmp avg function

94187c6

Log unfinished traceroute results

d636311

NullHypothesis reviewed Aug 9, 2022

View reviewed changes

reethikar added 10 commits August 13, 2022 13:56

Fix inconsistent var naming

7dd7051

Fix inconsistent capitalization, inline variables, don't pass variables that can be derived

Inline variables, fix name

0e14997

remove uuid from zerotrace obj

97859ce

Move sending TTL pkts to separate func

de1bbed

Abort signal for recvPackets goroutine

c08b5db

Fix checking for abort sig in recv Packets

957f67f

pcap handler errors return to caller

b5c2e3d

Fix use of multiple err messages

ffe573f

Remove excessive logging and log at caller, avoid special err variables

Input form to collect user details

f27acdb

Add a html form to have users submit contact info to help debug

Add loading graphic to measurement page

416b583

reethikar commented Aug 16, 2022

View reviewed changes

reethikar added 4 commits August 16, 2022 12:30

Fix abort signal issue in recvPackets

a89c1d2

Fix issue by reading from packet stream as a channel, do not check for isClientReached? in recvPackets, instead let sender figure it out and abort the receive goroutine

Merge branch 'refactor' into add-form

62d2dad

Check validity of uuid

942bcca

Merge branch 'refactor' into add-form

14d1076

reethikar added 7 commits August 16, 2022 14:48

Check uuid obtained from url param

cd4268f

Move form validation to utils

4e3ef0b

Fix email label

61ce7e7

Add comments for new functions

6b54c85

Comment about IPID and OS behavior

d4da564

Ran gofmt

3980d35

Merge branch 'refactor' into add-form

56c5382

reethikar requested a review from NullHypothesis August 16, 2022 22:29

Edit readme to add new endpoint

a34967a

NullHypothesis approved these changes Aug 17, 2022

View reviewed changes

Merge pull request #9 from brave-experiments/add-form

d268b0f

Input form to collect user details

reethikar merged commit ac898cf into main Aug 17, 2022

reethikar deleted the refactor branch August 17, 2022 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor main networking code #8

Refactor main networking code #8

reethikar commented Aug 5, 2022

reethikar commented Aug 8, 2022

NullHypothesis commented Aug 8, 2022

NullHypothesis left a comment

NullHypothesis Aug 8, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 17, 2022

NullHypothesis Aug 8, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 8, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 8, 2022

reethikar Aug 16, 2022 •

edited

Loading

NullHypothesis Aug 8, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 8, 2022

reethikar Aug 11, 2022 •

edited

Loading

NullHypothesis Aug 8, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 9, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 9, 2022

reethikar Aug 16, 2022

NullHypothesis Aug 9, 2022

reethikar Aug 15, 2022 •

edited

Loading

reethikar Aug 16, 2022

reethikar Aug 16, 2022 •

edited

Loading

NullHypothesis Aug 17, 2022

NullHypothesis commented Aug 17, 2022

reethikar commented Aug 17, 2022

Refactor main networking code #8

Refactor main networking code #8

Conversation

reethikar commented Aug 5, 2022

reethikar commented Aug 8, 2022

NullHypothesis commented Aug 8, 2022

NullHypothesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reethikar Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reethikar Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reethikar Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reethikar Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullHypothesis commented Aug 17, 2022

reethikar commented Aug 17, 2022

reethikar Aug 16, 2022 •

edited

Loading

reethikar Aug 11, 2022 •

edited

Loading

reethikar Aug 15, 2022 •

edited

Loading

reethikar Aug 16, 2022 •

edited

Loading