mirror of
https://github.com/weaveworks/scope.git
synced 2026-03-29 14:57:13 +00:00
234 lines
10 KiB
Markdown
234 lines
10 KiB
Markdown
Contributing To gopacket
|
|
========================
|
|
|
|
So you've got some code and you'd like it to be part of gopacket... wonderful!
|
|
We're happy to accept contributions, whether they're fixes to old protocols, new
|
|
protocols entirely, or anything else you think would improve the gopacket
|
|
library. This document is designed to help you to do just that.
|
|
|
|
The first section deals with the plumbing: how to actually get a change
|
|
submitted.
|
|
|
|
The second section deals with coding style... Go is great in that it
|
|
has a uniform style implemented by 'go fmt', but there's still some decisions
|
|
we've made that go above and beyond, and if you follow them, they won't come up
|
|
in your code review.
|
|
|
|
The third section deals with some of the implementation decisions we've made,
|
|
which may help you to understand the current code and which we may ask you to
|
|
conform to (or provide compelling reasons for ignoring).
|
|
|
|
Overall, we hope this document will help you to understand our system and write
|
|
great code which fits in, and help us to turn around on your code review quickly
|
|
so the code can make it into the master branch as quickly as possible.
|
|
|
|
|
|
How To Submit Code
|
|
------------------
|
|
|
|
gopacket uses the code.google.com Git version control system. If you want to
|
|
make a new change, you'll first have to get our code:
|
|
|
|
go get code.google.com/p/gopacket
|
|
cd $GOROOT/src/pkg/code.google.com/p/gopacket
|
|
git checkout -b <mynewfeature> # create a new branch to work from
|
|
... code code code ...
|
|
./gc # Run this to do local commits, it performs a number of checks
|
|
... code code code ...
|
|
./gc --benchmark # Run this whenever your commit could affect performance
|
|
|
|
Nw that you're in the gopacket code directory, you can start making your initial
|
|
change. PLEASE make sure you're using a new branch to develop whatever feature
|
|
you're working on.
|
|
|
|
Once you've got your code to a place where you're ready to have us look at it,
|
|
send an email to gopacket@googlegroups.com, detailing your change. We'll add
|
|
you as a committer, and you can upload your feature branch to code.google.com.
|
|
From there, the other folks working on gopacket can give you code reviews with
|
|
the code.google.com code review functionality.
|
|
|
|
The code review will generally be either emails or line-by-line reviews via
|
|
code.google.com. One or more folks might review your code. The review should
|
|
be considered "complete" when at least one of the project Owners (see
|
|
https://code.google.com/p/gopacket/people/list) gives you permission to merge to
|
|
master. At that point, you can merge to master yourself, or you can have one of
|
|
the other committers/owners do it for you.
|
|
|
|
When doing the final merge, please try to capture any interesting comments or
|
|
discussions that came up in code review. This will help future contributors be
|
|
able to find and reference those discussions later on.
|
|
|
|
To sum up:
|
|
|
|
* DO
|
|
+ Pull down the latest version.
|
|
+ Make a feature-specific branch.
|
|
+ Code using the style and methods discussed in the rest of this document.
|
|
+ Use the ./gc command to do local commits.
|
|
+ Send an email asking us to make you a committer (if you're new).
|
|
+ Push your new feature branch up to code.google.com.
|
|
+ Handle comments and requests from reviewers, pushing new commits up to
|
|
your feature branch as problems are addressed.
|
|
+ Get approval from a project Owner to merge to master.
|
|
+ Merge yourself, or have another Committer/Owner do it for you.
|
|
+ Put interesting comments and discussions into commit comments.
|
|
* DON'T
|
|
+ Push directly to master.
|
|
+ Push to someone else's branch without their permission.
|
|
+ Merge your own code to master without sign-off from others on the project.
|
|
+ Rebase (please merge)
|
|
* OPTIONAL
|
|
+ Review others' code as it comes in (politely :)
|
|
+ Keep contributing!
|
|
|
|
|
|
Coding Style
|
|
------------
|
|
|
|
* Go code must be run through 'go fmt'.
|
|
* Follow http://golang.org/doc/effective_go.html as much as possible.
|
|
+ In particular, http://golang.org/doc/effective_go.html#mixed-caps. Enums
|
|
should be be CamelCase, with acronyms capitalized (TCPSourcePort, vs.
|
|
TcpSourcePort or TCP_SOURCE_PORT).
|
|
* Bonus points for giving enum types a String() field.
|
|
* Any exported types or functions should have commentary
|
|
(http://golang.org/doc/effective_go.html#commentary)
|
|
|
|
|
|
Coding Methods And Implementation Notes
|
|
---------------------------------------
|
|
|
|
### Error Handling
|
|
|
|
Many times, you'll be decoding a protocol and run across something bad, a packet
|
|
corruption or the like. How do you handle this? First off, ALWAYS report the
|
|
error. You can do this either by returning the error from the decode() function
|
|
(most common), or if you're up for it you can implement and add an ErrorLayer
|
|
through the packet builder (the first method is a simple shortcut that does
|
|
exactly this, then stops any future decoding).
|
|
|
|
Often, you'll already have decode some part of your protocol by the time you hit
|
|
your error. Use your own discretion to determine whether the stuff you've
|
|
already decoded should be returned to the caller or not:
|
|
|
|
func decodeMyProtocol(data []byte, p gopacket.PacketBuilder) error {
|
|
prot := &MyProtocol{}
|
|
if len(data) < 10 {
|
|
// This error occurred before we did ANYTHING, so there's nothing in my
|
|
// protocol that the caller could possibly want. Just return the error.
|
|
return fmt.Errorf("Length %d less than 10", len(data))
|
|
}
|
|
prot.ImportantField1 = data[:5]
|
|
prot.ImportantField2 = data[5:10]
|
|
// At this point, we've already got enough information in 'prot' to
|
|
// warrant returning it to the caller, so we'll add it now.
|
|
p.AddLayer(prot)
|
|
if len(data) < 15 {
|
|
// We encountered an error later in the packet, but the caller already
|
|
// has the important info we've gleaned so far.
|
|
return fmt.Errorf("Length %d less than 15", len(data))
|
|
}
|
|
prot.ImportantField3 = data[10:15]
|
|
return nil // We've already added the layer, we can just return success.
|
|
}
|
|
|
|
In general, our code follows the approach of returning the first error it
|
|
encounters. In general, we don't trust any bytes after the first error we see.
|
|
|
|
### What Is A Layer?
|
|
|
|
The definition of a layer is up to the discretion of the coder. It should be
|
|
something important enough that it's actually useful to the caller (IE: every
|
|
TLV value should probably NOT be a layer). However, it can be more granular
|
|
than a single protocol... IPv6 and SCTP both implement many layers to handle the
|
|
various parts of the protocol. Use your best judgement, and prepare to defend
|
|
your decisions during code review. ;)
|
|
|
|
### Performance
|
|
|
|
We strive to make gopacket as fast as possible while still providing lots of
|
|
features. In general, this means:
|
|
|
|
* Focus performance tuning on common protocols (IP4/6, TCP, etc), and optimize
|
|
others on an as-needed basis (tons of MPLS on your network? Time to optimize
|
|
MPLS!)
|
|
* Use fast operations. See the toplevel benchmark_test for benchmarks of some
|
|
of Go's underlying features and types.
|
|
* Test your performance changes! You should use the ./gc script's --benchmark
|
|
flag to submit any performance-related changes. Use pcap/gopacket_benchmark
|
|
to test your change against a PCAP file based on your traffic patterns.
|
|
* Don't be TOO hacky. Sometimes, removing an unused struct from a field causes
|
|
a huge performance hit, due to the way that Go currently handles its segmented
|
|
stack... don't be afraid to clean it up anyway. We'll trust the Go compiler
|
|
to get good enough over time to handle this. Also, this type of
|
|
compiler-specific optimization is very fragile; someone adding a field to an
|
|
entirely different struct elsewhere in the codebase could reverse any gains
|
|
you might achieve by aligning your allocations.
|
|
* Try to minimize memory allocations. If possible, use []byte to reference
|
|
pieces of the input, instead of using string, which requires copying the bytes
|
|
into a new memory allocation.
|
|
* Think hard about what should be evaluated lazily vs. not. In general, a
|
|
layer's struct should almost exactly mirror the layer's frame. Anything
|
|
that's more interesting should be a function. This may not always be
|
|
possible, but it's a good rule of thumb.
|
|
* Don't fear micro-optimizations. With the above in mind, we welcome
|
|
micro-optimizations that we think will have positive/neutral impacts on the
|
|
majority of workloads. A prime example of this is pre-allocating certain
|
|
structs within a larger one:
|
|
|
|
type MyProtocol struct {
|
|
// Most packets have 1-4 of VeryCommon, so we preallocate it here.
|
|
initialAllocation [4]uint32
|
|
VeryCommon []uint32
|
|
}
|
|
|
|
func decodeMyProtocol(data []byte, p gopacket.PacketBuilder) error {
|
|
prot := &MyProtocol{}
|
|
prot.VeryCommon = proto.initialAllocation[:0]
|
|
for len(data) > 4 {
|
|
field := binary.BigEndian.Uint32(data[:4])
|
|
data = data[4:]
|
|
// Since we're using the underlying initialAllocation, we won't need to
|
|
// allocate new memory for the following append unless we more than 16
|
|
// bytes of data, which should be the uncommon case.
|
|
prot.VeryCommon = append(prot.VeryCommon, field)
|
|
}
|
|
p.AddLayer(prot)
|
|
if len(data) > 0 {
|
|
return fmt.Errorf("MyProtocol packet has %d bytes left after decoding", len(data))
|
|
}
|
|
return nil
|
|
}
|
|
|
|
### Slices And Data
|
|
|
|
If you're pulling a slice from the data you're decoding, don't copy it. Just
|
|
use the slice itself.
|
|
|
|
type MyProtocol struct {
|
|
A, B net.IP
|
|
}
|
|
func decodeMyProtocol(data []byte, p gopacket.PacketBuilder) error {
|
|
p.AddLayer(&MyProtocol{
|
|
A: data[:4],
|
|
B: data[4:8],
|
|
})
|
|
return nil
|
|
}
|
|
|
|
The caller has already agreed, by using this library, that they won't modify the
|
|
set of bytes they pass in to the decoder, or the library has already copied the
|
|
set of bytes to a read-only location. See DecodeOptions.NoCopy for more
|
|
information.
|
|
|
|
### Enums/Types
|
|
|
|
If a protocol has an integer field (uint8, uint16, etc) with a couple of known
|
|
values that mean something special, make it a type. This allows us to do really
|
|
nice things like adding a String() function to them, so we can more easily
|
|
display those to users. Check out layers/enums.go for one example, as well as
|
|
layers/icmp.go for layer-specific enums.
|
|
|
|
When naming things, try for descriptiveness over suscinctness. For example,
|
|
choose DNSResponseRecord over DNSRR.
|