Subscribe to receive notifications of new posts:

Making code better with reviews

2014-07-02

2 min read

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.

Cloudflare's connectivity cloud protects entire corporate networks, helps customers build Internet-scale applications efficiently, accelerates any website or Internet application, wards off DDoS attacks, keeps hackers at bay, and can help you on your journey to Zero Trust.

Visit 1.1.1.1 from any device to get started with our free app that makes your Internet faster and safer.

To learn more about our mission to help build a better Internet, start here. If you're looking for a new career direction, check out our open positions.
ProgrammingDevelopersDNSReliability

Follow on X

Cloudflare|@cloudflare

Related posts

September 27, 2024 1:00 PM

AI Everywhere with the WAF Rule Builder Assistant, Cloudflare Radar AI Insights, and updated AI bot protection

This year for Cloudflare’s birthday, we’ve extended our AI Assistant capabilities to help you build new WAF rules, added new AI bot & crawler traffic insights to Radar, and given customers new AI bot blocking capabilities...

September 27, 2024 1:00 PM

Our container platform is in production. It has GPUs. Here’s an early look

We’ve been working on something new — a platform for running containers across Cloudflare’s network. We already use it in production, for AI inference and more. Today we want to share an early look at how it’s built, why we built it, and how we use it ourselves. ...