search

m-lab / ndt

Network Diagnostic Tool
Other
11 stars 7 forks source link

Pull request 4 of N, N = 4. #6

Closed pboothe closed 9 years ago

pboothe commented 9 years ago

Adds support for SSL to the NDT server.

Final one.

mtlynch commented 9 years ago

Finished this round of review

pboothe commented 9 years ago

PTAL

mtlynch commented 9 years ago

Can we rename to arg_index

We can, but i is the standard index variable name, so it seems kinda silly.

I think i is alright when it's immediately obvious what i is an index of, but I don't feel like it's obvious here.

mtlynch commented 9 years ago

Can we just initialize the whole struct to {0} at declaration time?

We can, but all we care about is that the pid is 0.

Optional: Eh, either way. I thought you might like to save a line.

pboothe commented 9 years ago

I'll change it, but, as we discussed in person, under slight duress. :)

mtlynch commented 9 years ago

Suggest switching parameter order so it's always hostname, then port.

Why? It's port then hostname everywhere in this file.

This is counter-intuitive to me because I've never seen it ordered that way in a networking API. To me, it's akin to frob_time(seconds, minutes, hours). It also clashes with patterns in other files:

src/web100srv_unit_tests.c:65:  sprintf(command_line, "./web100clt --name=%s --port=%d", hostname, port);

src/web100srv_unit_tests.c:96:          "%s node_tests/ndt_client.js --server %s --port %d --tests %d",

src/web100srv_unit_tests.c-157-  sprintf(openssl_client_command_line, "echo | openssl s_client -connect %s:%d",
src/web100srv_unit_tests.c:158:          hostname, port);

src/test_c2s_clt.c:115:    I2AddrSetPort(sec_addr, c2sport);  // set port value
mtlynch commented 9 years ago

Some follow up comments after the last PTAL but nothing blocking. LGTM.