Note: This is the first time I've written anything whatsoever in straight C. In other words: I have no idea what I'm doing.

I recently had a task that involved temporarily relaying responses from one server, through a web-facing server, and then on to the client. Along the way, the relay server had to dynamically add an HTTP header of its own to the relayed response. Came up with a quick and dirty scripted solution, but it worked for the duration it had to.

Still, I thought it might be a good exercise for getting to know a little C, just for fun, since something like this would benefit from being written closer to the metal.

So below is a program (I call it headshape for lack of a better name) where you pipe in (say, from a curl command) an HTTP response including headers, and the program will add/remove/modify one of those headers, and pass on the rest on as-is. Given no arguments, stdin will just pass to stdout.

So it does a bit of HTTP parsing to find the right header - or add it if it's not there - and to figure out what the response code is. Only responses with a 2xx status will be altered, 1xx responses will be ignored, and 3xx and above will cause it to just forward all remaining data unaltered (this partly an arbitrary choice; it's just a code exercise after all). It also defaults to forwarding everything as-is if it sees something it can't make sense of.

Below is the code. Much to my surprise it seems to work exactly as intended but, again, I've never written C code, so it's no doubt horrible. Any and all tips are welcome.

1 Answer
1

Use STDIN_FILENO

Instead of fileno(stdin) you can use the preprocessor symbol STDIN_FILENO, defined in <unistd.h> which you have already included.

should_parse should be declared bool

Assuming you're using a compiler that isn't decades old (bool was added in the c99 standard), you can include <stdbool.h> and use bool for the type of should_parse. It also allows you to use the values of true and false within the code to make the intention of that flag a little more explicit.

The same is true for line_continued, the return value of isBoundary, and isDelimited and the second argument of isDelimited.

target_header should be const and argv should not

Your target_header variable should be declared const because its contents are not altered by the program. If you make that change, you can also remove the cast from the line

target_header = (const char *)argv[1];

However, you wouldn't actually need that cast anyway if you had declared main as:

int main(int argc, char *argv[])

It does seem like that ought to be const *argv but that's counter to what the C standard says. In section 5.1.2.2.1, it says that:

the strings pointed to by the argv array shall be modifiable by the program

which implies that they're not const.

Don't abuse fflush

You probably don't need to call fflush after every fwrite. The stream will automatically flush when the file is closed, which also happens automatically when main ends.

Make loop exits explicit

In passthru(), instead of using while(1) and then using a break it would be more clear to write it like this:

Simplify isBoundary

There is no need to also compare their lengths, since that's already implicit in strcmp.

Calculation of payload_line is complex

The payload_line variable size is calculated, and then malloc'd and then copied, but it's only used a few other places. What you've got isn't wrong, but it might be worthwhile instead allocating a fixed size and then using snprintf to populate the string. That would collapse the dozen lines used for that calculation to the much simpler single line:

Refactor getHeaderName

This works (and even some ancient library functions were done this way) but it's not thread-safe and the design can easily be improved. Instead, it's better to have the calling function allocate a buffer and pass it in, along with the length rather than pointing to an internal static buffer.

Refactor isDelimited

In a similar vein, your isDelimited function is really only checking for two particular bytes, so it would be more clear and slightly faster to check for them directly instead of constructing a copy in memory and then using strcmp.

Great tips, thanks! Re bool: was trying some things out using ints, and it stuck; will definitely fix that. argv: I used my editor's C template and didn't ask questions. Good to know the const isn't required. fflush: I smelled that myself, but I also figured I'd have many Mbits/s streaming through, so maybe better to get stuff flushed asap? Loop exit: Guilty of googling and copy/pasting without thinking! isBoundary: Ah, of course! Should've seen that myself. payload_line: I could smell that code from a mile away, but I didn't know a better way to do it. Now I do - thanks!
–
FlambinoMay 4 '14 at 23:55

1

It's a pretty good attempt for a first foray into C. You did well, despite all the nit picking!
–
EdwardMay 4 '14 at 23:58

1

Oh, and in my defense, using malloc and free made me feel like I was really writing C code after all these years of languages with GC ;)
–
FlambinoMay 4 '14 at 23:58

Thanks! And hey, nit-picking is what I want in a review! I mean, it'd be pretty cool if my very first stab at C was a gleaming example of perfection, but let's just say I didn't expect that to be the case :)
–
FlambinoMay 5 '14 at 0:00

Just noticed your update - thanks again! Good point about the construction/signature of getHeaderName in particular. I definitely see your point (and I recognize the pass-in-a-buffer approach, now that you mention it). Great stuff.
–
FlambinoMay 5 '14 at 15:01