In the past we've written about how CloudFlare isn't afraid to rip out and replace chunks of code that have proved to be hard to maintain or have simply reach end of life. For example, we wrote a brand new DNS server and replaced our old DNS infrastructure with it. Doing so was greatly helped by two things: a large test suite (that keeps growing) and code reviews.
Recently, I was working on that same DNS server when I needed to understand and change the following code:
if raw[2]&0x86 != 0 || raw[3]&0x4f != 0 { return false }
raw is a []byte containing a DNS packet and this code is part of a filter that throws away packets that contain invalid flags. In this case the code is throwing away query packets that contain flags that are only used in responses.
It took me a while to grok which flags 0x86 and 0x4f were masking and having understood them I decided to update the code with the following comment:
// DNS Header looks like this: // // 1 1 1 1 1 1 // 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 // +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+ // | ID | // +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+ // |QR| OPCODE |AA|TC|RD|RA| Z|AD|CD| RCODE | // +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+ // // So ID is in raw[0:1], raw[2] contains QR, OPCODE, AA TC and RD. raw[3] // contains RA, Z, AD, CD and RCODE. // // 0x86 in raw[2] corresponds to masking on the QR, AA and TC // 0x4f in raw[3] corresponds to masking on the Z and RCODE
if raw[2]&0x86 != 0 || raw[3]&0x4f != 0 { return false }
I figured that I'd save the next person time in dissecting those values since the actual time consuming item was figuring out what they meant; writing a comment dumping my current memory state was quick. And it meant leaving code in better shape than when I first looked at it.
I sent that code (and bunch of other change) off for review and my colleague Marek came back with:
> 0x86 in raw[2] corresponds to masking on the QR, AA and TC
Nah, how about making it: flags := raw[2] <<8 | raw[3] and then checking it against or-ed flags, like (flags & (0x1 | 0x2 | 0x4 ....) and letting the compiler to come up with magic 0x86 :)
That made sense to me and I changed the code to:
// DNS Header looks like this: // // 1 1 1 1 1 1 // 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 // +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+ // | ID | // +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+ // |QR| OPCODE |AA|TC|RD|RA| Z|AD|CD| RCODE | // +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+ // // So ID is in raw[0:1], raw[2] contains QR, OPCODE, AA TC and RD. raw[3] // contains RA, Z, AD, CD and RCODE.
const QR byte = 0x80const AA byte = 0x04const TC byte = 0x02
const Z byte = 0x40const RCODE byte = 0x0f
if raw[2]&(QR|AA|TC) != 0 || raw[3]&(Z|RCODE) != 0 {return false}
That's much clearer and requires no thinking on the part of the next person who reads or maintains the code. Along the way Marek had also pointed me to some information about the AD and CD bits which meant that I'd improved the code and learnt something new.
At that point the Approve button was hit on the pull request and the code was available to be merged into the mainline. All our code goes through this type of process to improve its quality and security, and it's an essential part of our PCI certification. Our SRE team follows a similar process for configuration changes.
Even this blog post got peer review.
If you're interested in working on CloudFlare-scale code, and making things better in any of the languages we work with Lua, Go, C, PHP, Python, JavaScript, ... then we're hiring in London and San Francisco.