Combine TLS support and raw socket support into a single process. Thi…

[pboothe]

Makes it so that a single server can support both TLS and unencrypted connections. Alters the test_queueing unit test to queue up raw, websocket, and TLS websocket tests all in the same queue. All tests pass.

[pboothe]

src/usage.c, line 76 [r1] (raw file): Fixed in the next commit - thanks!

---

Comments from the review on Reviewable.io

[pboothe]

_src/web100srv_unit_tests.c, line 89 [r1] (raw file):_ Moved downwards to the bottom of the file, and it was changed to include a mix of raw, websocket, and secure websocket clients.

---

Comments from the review on Reviewable.io

[pboothe]

_src/web100srv_unit_tests.c, line 89 [r1] (raw file):_ Done.

---

Comments from the review on Reviewable.io

[pboothe]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

_src/web100srv_unit_tests.c, line 15 [r1] (raw file):_ Refactored from run_ssl_test

---

_src/web100srv_unit_tests.c, line 119 [r1] (raw file):_ Refactored from test_node

---

_src/web100srv_unit_tests.c, line 336 [r1] (raw file):_ Tests are now ordered by runtime, with the tests that take the longest happening last.

---

Comments from the review on Reviewable.io

[pboothe]

Added comments on reviewable to try and give context to changes in web100srv_unit_test.c

Feel free to ignore if they are not helpful.

[mtlynch]

Reviewed 1 of 3 files at r1. Review status: 1 of 3 files reviewed at latest revision, 10 unresolved discussions.

---

src/usage.c, line 77 [r1] (raw file): The wording here is kind of confusing. It seems to say it can't do a test if the test requires the server to open a socket and the test requires the --certificate and --private-key options (which I don't think is what we mean). Also I hate the passive voice and think it is to be avoided wherever possible.

Original

Use this port for TLS sockets to conduct the tests. Note: TLS tests can't do tests which require the server to open a socket to the client (MID, SFW), and also require the --certificate and --private-key options be set. If this is unset, TLS is not used.

Suggested

Use this port to conduct the tests over TLS sockets. This option requires requires the --certificate and --private-key options be set. If this option is unset, NDT does not use TLS. Note: NDT cannot perform any tests over TLS if the test requires the server to open a socket to the client (MID, SFW).

---

src/web100srv.c, line 1553 [r1] (raw file): Need to update the return part.

---

src/web100srv.c, line 1555 [r1] (raw file): Need to document fd_max

---

src/web100srv.c, line 1891 [r1] (raw file): It seems like this is ignored if ssl_context is NULL. Can we document this?

---

src/web100srv.c, line 1892 [r1] (raw file): Can we specify that these are non-TLS clients?

---

src/web100srv.c, line 1899 [r1] (raw file): Should be underscore separated, suggest fd_max for consistency with the function above.

---

_src/web100srv.c, line 2323 [r1] (raw file):_ It's weird that one of these is a flag name and the other two are friendly names. Consider making them all the flag names.

---

src/web100srv.c, line 2398 [r1] (raw file): What happened to this?

---

src/web100srv.c, line 2428 [r1] (raw file): Shouldn't the first parameter be NULL since it does not yet exist? This makes it explicit that we're not passing anything to CreateListenSocket.

---

src/web100srv.c, line 2429 [r1] (raw file): Can we do the ternary outside the if? This is a lot of logic to cram into a single if condition.

---

Comments from the review on Reviewable.io

[mtlynch]

Reviewed 1 of 3 files at r1. Review status: 2 of 3 files reviewed at latest revision, 10 unresolved discussions.

---

Comments from the review on Reviewable.io

[mtlynch]

Review status: 2 of 3 files reviewed at latest revision, 15 unresolved discussions.

---

_src/web100srv_unit_tests.c, line 15 [r1] (raw file):_ Whee refactoring!

---

_src/web100srv_unit_tests.c, line 15 [r1] (raw file):_ I don't understand what we mean by "passed-in specs". We're just passing in filenames.

---

_src/web100srv_unit_tests.c, line 29 [r1] (raw file):_ Can we make these snprintf because now if the params are long paths, we overflow the buffer.

---

_src/web100srv_unit_tests.c, line 287 [r1] (raw file):_ We're repeating this code a lot (in the TLS and non-TLS tests). Suggest refactoring to a function like

choose_random_ports(int *port, int *tls_port) {
  srandom(time(NULL));
  port = (random() % 30000) + 1024;
  if (tls_port == NULL) return;
  do {
    tls_port = (random() % 30000) + 1024;
  } while (tls_port == port);
}

---

_src/web100srv_unit_tests.c, line 336 [r1] (raw file):_ Can add a comment explaining this?

---

Comments from the review on Reviewable.io

[pboothe]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

src/usage.c, line 77 [r1] (raw file): Done.

---

src/usage.c, line 110 [r3] (raw file): One cleanup - I added this option a long time ago but failed to document it in the usage.

---

src/web100srv.c, line 1553 [r1] (raw file): Done.

---

src/web100srv.c, line 1555 [r1] (raw file): Done.

---

src/web100srv.c, line 1891 [r1] (raw file): Done.

---

src/web100srv.c, line 1892 [r1] (raw file): Done.

---

src/web100srv.c, line 1899 [r1] (raw file): Done.

---

_src/web100srv.c, line 2323 [r1] (raw file):_ Done.

---

src/web100srv.c, line 2398 [r1] (raw file): It was dead code. Has been forever, I only noticed that it was dead just now when I went to create a parallel initialization for tls_listenfd.

---

src/web100srv.c, line 2428 [r1] (raw file): Changed (both here and above)

---

src/web100srv.c, line 2429 [r1] (raw file): Done.

---

_src/web100srv_unit_tests.c, line 15 [r1] (raw file):_ The filenames we pass in have XXXXX in them for mkstemp to replace with other chars.

---

_src/web100srv_unit_tests.c, line 29 [r1] (raw file):_ done, also bumped the buffer size

---

_src/web100srv_unit_tests.c, line 287 [r1] (raw file):_ Done, but without making the memory point to a random location. :)

---

_src/web100srv_unit_tests.c, line 336 [r1] (raw file):_ Done.

---

Comments from the review on Reviewable.io

[mtlynch]

LGTM

---

Reviewed 3 of 3 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

_src/web100srv.c, line 2013 [r3] (raw file):_ Suggest something more descriptive like socket_buffer_size

---

_src/web100srv_unit_tests.c, line 339 [r3] (raw file):_ Suggest in ascending order of expected runtime

---

Comments from the review on Reviewable.io