search

m-lab / ndt

Network Diagnostic Tool
Other
11 stars 7 forks source link

SSL support in NDT, merged with extended C2S and S2C tests. #15

Closed pboothe closed 9 years ago

pboothe commented 9 years ago

A clean merge! Please review and I will fix things up as needed.

Review on Reviewable

stephen-soltesz commented 9 years ago

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

src/test_c2s_clt.c, line 133 [r1] (raw file): is this diagnostic? should it be placed within a log_printf() with the appropriate debug level?

src/test_c2s_srv.c, line 29 [r1] (raw file): is the live_connections parameter used?

src/test_c2s_srv.c, line 32 [r1] (raw file): read_rv appears unused?

src/test_c2s_srv.c, line 42 [r1] (raw file): Under what circumstances would a peer renegotiate the connection? https://www.openssl.org/docs/manmaster/ssl/SSL_read.html

I can imagine this impacting performance of this connection. In our case, the peer would be a browser. Is this a concern?

src/test_c2s_srv.c, line 116 [r1] (raw file): is live_connections needed? I don't see that it's read anywhere (after being set on line ~305).

src/test_c2s_srv.c, line 297 [r1] (raw file): will (c2s_conn[i].socket == -1) ever be evaluated if the condition above 'continue's when c2s_conn[i].socket <= 0?

Should this block be before? or an internal check within the above block?s

src/test_c2s_srv.c, line 455 [r1] (raw file): I am not certain that c2s_conn[streamsNum-1].socket == maxFdn in all cases.

It looks like the fork() occurs after accept()'s; is it possible for two clients to use multi stream connections, with one calling accept while the second's connections are being closed by the parent, such that the socket fd's returned are not sequential?

src/test_c2s_srv.c, line 456 [r1] (raw file): What if errno != EINTR?

src/test_s2c_srv.c, line 562 [r1] (raw file): What do you think of placing this block into a helper function like "read_ready_streams"? Maybe s2cWriteWorker could call this general method also.

src/test_s2c_srv.c, line 583 [r1] (raw file): This condition looks redundant.

src/web100srv.c, line 428 [r1] (raw file): As discussed in person, I understand these were not your changes.

Due to 'n == 5' there is ambiguity between "administrator_view" and "admin_file".

src/web100srv.c, line 462 [r1] (raw file): As discussed in person, I understand these were not your changes.

See also "log*".

In general, the lengths of these strings are wrong throughout.

src/web100srv.c, line 493 [r1] (raw file): As discussed in person, I understand these were not your changes.

"logDue to 'n == 5' there is ambiguity between "diable_FIFO" & ""addisablesnaps".

Comments from the review on Reviewable.io

pboothe commented 9 years ago

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

src/test_c2s_clt.c, line 133 [r1] (raw file): Should be deleted. Will be deleted.

src/test_c2s_srv.c, line 42 [r1] (raw file): It's a little bit of a concern, but we can do nothing about it.

src/test_s2c_srv.c, line 91 [r1] (raw file): Less random comment formatting.

src/test_s2c_srv.c, line 562 [r1] (raw file): read_ready_streams was a useful refactoring to discover the bug, but was itself also a nontrivial source of bugs. I'd prefer to touch this minimally.

src/unit_testing.c, line 23 [r1] (raw file): Cleaning up the test output.

src/web100srv.c, line 164 [r1] (raw file): Added this new global and corresponding command line parameter to (optionally) disable the extended tests which both Matt and StephenS agree exist only to cover up

src/web100srv.c, line 207 [r1] (raw file): Cleanup. The only new arg is disable_extended_tests, which does what it says.

src/web100srv.c, line 2273 [r1] (raw file): These numbers need to match the numbers in the command line args array

src/web100srv.c, line 2439 [r1] (raw file): This snuck in by accident and should never have been in the merge in the first place.

src/web100srv.h, line 150 [r1] (raw file): Already been reviewed, just snuck in due to git history fluctuation. Eliminates the possibility of multiple processes attempting to use the ctl channel, which would break SSL clients.

src/web100srv_unit_tests.c, line 17 [r1] (raw file): Changed to support arbitrary extra args rather than just a boolean masquerading as an int.

src/web100srv_unit_tests.c, line 314 [r1] (raw file): Set to -1 to reduce test spam.

src/websocket_unit_tests.c, line 413 [r1] (raw file): Reduce test spam

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

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

src/test_c2s_srv.c, line 27 [r1] (raw file): Can we add documentation for the function and parameters?

src/test_c2s_srv.c, line 30 [r1] (raw file): buff_size is unused

src/test_c2s_srv.c, line 31 [r1] (raw file): double seems strange. Why not uint64_t?

src/test_c2s_srv.c, line 34 [r1] (raw file): Suggest current_bytes_read (at minimum should be snake case)

src/test_c2s_srv.c, line 37 [r1] (raw file): Dominant pattern in this file seems to be single space after period:

$ egrep "//.*\.\s+" src/test_c2s_srv.c
      // Use read or SSL_read in their raw forms.  We want this to go as fast
  // Determine port to be used. Compute based on options set earlier
    // socket interrupted. continue waiting for activity on socket
    if (msgretvalue < 0)  // other socket errors. exit
    if (j == (RETRY_COUNT*streamsNum - 1))  // retry exceeded. exit
      // Proceed.  Note the new socket fd - recvsfd- used in the throughput test
  // Get address associated with the throughput test. Used for packet tracing
  // Get tcp_stat connection. Used to collect tcp_stat variable statistics
  // set up packet tracing. Collected data is used for bottleneck link
  // full directory but fopen needs full path. Else, it could have gone into
      // select interrupted. Continue waiting for activity on socket
  // get receiver side Web100 stats and write them to the log file. close

src/test_c2s_srv.c, line 40 [r1] (raw file): Can we replace 8192 with a named constant?

src/test_c2s_srv.c, line 45 [r1] (raw file): This doesn't appear to be correct for TLS because we need to call SSL_get_error:

https://www.openssl.org/docs/manmaster/ssl/SSL_read.html Call SSL_get_error() with the return value ret to find out the reason.

src/test_c2s_srv.c, line 46 [r1] (raw file): Trailing whitespace

src/test_c2s_srv.c, line 49 [r1] (raw file): Comment seems extraneous

src/test_c2s_srv.c, line 117 [r1] (raw file): Suggest c2s_conns to convey that this represents multiple connections (ditto for other places we use this variable name)

src/test_c2s_srv.c, line 122 [r1] (raw file): Can we clarify the naming between test_duration and testDuration?

src/test_c2s_srv.c, line 136 [r1] (raw file): These seem unnecessary. They're only read in one place and have the values hardcoded right beforehand. Can we change this:

      procstatusenum = PROCESS_STARTED;
      proctypeenum = CONNECT_TYPE;
      protolog_procstatus(testOptions->child0, testids, proctypeenum,
                          procstatusenum, c2s_conn[i].socket);

to this:

      protolog_procstatus(testOptions->child0, testids, CONNECT_TYPE,
                          PROCESS_STARTED, c2s_conn[i].socket);

src/test_c2s_srv.c, line 267 [r1] (raw file): This is now extraneous

src/test_c2s_srv.c, line 273 [r1] (raw file): Can we refactor this loop to a separate function (preferably two, one for inner loop one for outer loop)? I'd really prefer to not bloat test_c2s further

src/test_c2s_srv.c, line 273 [r1] (raw file): Suggest different names for i and j since they're not very clear. i -> stream_index and j -> retry_count?

src/test_c2s_srv.c, line 278 [r1] (raw file): Decrementing the iterator in the middle of a for loop is really hacky. Can we rewrite this to be a while loop?

src/test_c2s_srv.c, line 289 [r1] (raw file): Comment needs update

src/test_c2s_srv.c, line 324 [r1] (raw file): (not your fault, but...) Shouldn't this be a fatal error?

src/test_c2s_srv.c, line 326 [r1] (raw file): trailing whitespace

src/test_c2s_srv.c, line 330 [r1] (raw file): This should be within an if (errno) { ... }

src/test_c2s_srv.c, line 343 [r1] (raw file): We're repeating this loop in several places. Can we refactor to a close_multiple_connections function?

src/test_c2s_srv.c, line 449 [r1] (raw file): Can we replace 10 with a named constant?

src/test_c2s_srv.c, line 485 [r1] (raw file): (minor) missing trailing period

src/test_c2s_srv.c, line 493 [r1] (raw file): This is essentially the same code as 443-446. Can we refactor to a helper function like:

size_t connections_to_fd_set(Connection *connections, size_t connections_length) {
  size_t active_connections;
  fd_set fds;
  FD_ZERO(&fds);
  for (size_t i = 0; i < connections_length; i++) {
    if (connections[i].socket > 0) {
      active_connections++;
      FD_SET(connections[i].socket, &fds);
    }
  }
  return active_connections;
}

src/websocket_unit_tests.c, line 413 [r1] (raw file): Why are we changing this?

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

Reviewed 7 of 19 files at r1. Review status: 8 of 19 files reviewed at latest revision, 49 unresolved discussions.

src/test_s2c_srv.c, line 296 [r1] (raw file): Same note as in c2s, can we give better names to i, j, make the outer loop a while instead of a for because it doesn't match the pattern of a for loop.

src/test_s2c_srv.c, line 322 [r1] (raw file): If we're touching these lines anyway, we should remove the tabs.

src/test_s2c_srv.c, line 328 [r1] (raw file): Shouldn't this be i >= streamsNum? And shouldn't it be a break?

src/test_s2c_srv.c, line 333 [r1] (raw file): Same note as in c2s, we don't need to save these enums as variables.

src/test_s2c_srv.c, line 562 [r1] (raw file): I agree with stephen-soltesz@. We shouldn't be adding a dozen lines to a function that's already 800+ lines long

src/test_s2c_srv.c, line 806 [r1] (raw file): Why are we deleting this?

src/websocket_unit_tests.c, line 413 [r1] (raw file): Made this comment before you added yours, published before I got to it. Disregard.

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

Reviewed 8 of 19 files at r1. Review status: 16 of 19 files reviewed at latest revision, 57 unresolved discussions.

src/web100srv.c, line 164 [r1] (raw file): Cover up what?

src/web100srv.c, line 207 [r1] (raw file): refresh is out of order in this group

src/web100srv.c, line 221 [r1] (raw file): logfacility appears twice

src/web100srv.c, line 1401 [r1] (raw file): Can we add documentation here?

src/web100srv.c, line 1467 [r1] (raw file): Can we make this memset(webVarsValuesLog, 0, sizeof(webVarsValuesLog));?

src/web100srv.c, line 1469 [r1] (raw file): Can we refactor 1469-1471 to a helper function? I don't want to add additional variables to this function.

src/web100srv.c, line 1469 [r1] (raw file): Why dir instead of like log_filename?

src/web100srv.c, line 1471 [r1] (raw file): Can we add a check that sizeof(meta.web_variables_log) >= (strlen(dir) + 1) and add a null terminator to meta.web_variables_log? (current implementation assumes the buffer is zeroed)

src/web100srv.c, line 2000 [r1] (raw file): These seem redundant given that the memset zeroes all fields.

src/web100srv.c, line 2003 [r1] (raw file): Can we make this memset(options.c2s_logname, 0, sizeof(options.c2s_logname));?

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

Reviewed 3 of 19 files at r1. Review status: all files reviewed at latest revision, 73 unresolved discussions.

src/web100srv_unit_tests.c, line 15 [r1] (raw file): We should mention that args can be NULL to indicate no additional args

src/web100srv_unit_tests.c, line 17 [r1] (raw file): args and args_for_exec are kind of confusing names. Suggest args->additional_args, args_for_exec->args

src/web100srv_unit_tests.c, line 17 [r1] (raw file): Why did we need to support arbitrary extra args in order to merge multiport and TLS?

src/web100srv_unit_tests.c, line 19 [r1] (raw file): I don't understand this comment. args_for_exec[0] is not NULL

src/web100srv_unit_tests.c, line 23 [r1] (raw file): Don't understand this comment, where is 32767? Edit: Now I get it. Suggest rewriting to port can be at most 5 digits (max value is 32767) + NULL terminator

src/web100srv_unit_tests.c, line 30 [r1] (raw file): Can we initialize to 0 on creation?

src/web100srv_unit_tests.c, line 41 [r1] (raw file): server port -> server pid?

src/web100srv_unit_tests.c, line 45 [r1] (raw file): Can we get rid of rv and make this:

ASSERT(waitid(P_PID, server_pid, &server_status, WEXITED | WSTOPPED | WNOHANG) == 0 && server_status.si_pid == 0, "The server did not start successfully");

src/web100srv_unit_tests.c, line 52 [r1] (raw file): Can we make both client_options and server_options into char **? It's confusing that they have matching names and identical purposes but different types.

src/web100srv_unit_tests.c, line 67 [r1] (raw file): Suggest reversing the logic to make it less negative-based: client_options == NULL ? "" : client_options or client_options ? client_options : ""

src/web100srv_unit_tests.c, line 75 [r1] (raw file): Can we add comments for this test and test_e2e? Can we keep the e2e test that isn't multiport (as we had previously)?

src/web100srv_unit_tests.c, line 134 [r1] (raw file): Why is this fprintf instead of log_print?

src/web100srv_unit_tests.c, line 134 [r1] (raw file): Can not -> Cannot

src/web100srv_unit_tests.c, line 208 [r1] (raw file): (no action required) This file shows 300+ lines of changes, but <100 lines changed since this code was last reviewed (at 86f690), and most of the changes since that review are very simple. In the future please be mindful of reviewers' time and merge carefully so as not to bloat the diff with changes that have already been reviewed.

src/web100srv_unit_tests.c, line 286 [r1] (raw file): What is this function? It looks like a test that was never actually written.

src/web100srv_unit_tests.c, line 310 [r1] (raw file): to pass -> to run

src/web100srv_unit_tests.c, line 317 [r1] (raw file): (grammar nitpick) extraneous comma

Comments from the review on Reviewable.io

stephen-soltesz commented 9 years ago

Review status: all files reviewed at latest revision, 73 unresolved discussions.

src/test_c2s_srv.c, line 42 [r1] (raw file): I believe there are several approaches we can take.

The ideas that I can imagine are: learning under what circumstances our clients might request renegotiation and if it will never occur from our clients adding a comment that this is a non-issue to signal that we have investigated this, checking the number of renegotiations that have occurred after the test is complete and aborting / invalidating the test for anything > 1 (e.g. SSL_num_renegotiations), or disabling renegotiations in the server so that renegotiation cannot occur in the first place.

My thought is that our existing infrastructure is complex enough to cause impacts on our data under some circumstances. NDT-SSL will introduce additional complexity which will make future analysis of anomalies more difficult because the variables that impacted performance will no longer be present. One strategy for limiting the scope of potential impacts is pinning features. Is this a prudent approach? What else would be helpful?

Comments from the review on Reviewable.io

pboothe commented 9 years ago

PTAL

Review status: 14 of 18 files reviewed at latest revision, 23 unresolved discussions.

src/test_c2s_srv.c, line 27 [r1] (raw file): Done.

src/test_c2s_srv.c, line 29 [r1] (raw file): Nope. Vestigial, and now deleted.

src/test_c2s_srv.c, line 30 [r1] (raw file): Fixed.

src/test_c2s_srv.c, line 31 [r1] (raw file): I thought the same thing, and so tried changing it. Doing so broke lots of other things, and so rather than chasing that rabbit hole I changed it back.

src/test_c2s_srv.c, line 32 [r1] (raw file): Deleted.

src/test_c2s_srv.c, line 34 [r1] (raw file): Changed

src/test_c2s_srv.c, line 37 [r1] (raw file): Done.

src/test_c2s_srv.c, line 40 [r1] (raw file): Should have been buff_size all along. Fixed.

src/test_c2s_srv.c, line 42 [r1] (raw file): Renegotiations can be initialized by either side, unfortunately. I am comfortable with characterizing our tesing as testing "goodput" rather than throughput, and "goodput" is affected by renegotiations (as it should be). The circumstances under which renegotiation is requested are fluid, and so that approach is doomed. I am open to the idea of adding SSL_num_renegotiations to the output returned by TEST_META however. Can we resolve this with a TODO(add the number of SSL renegotiations to the META variables)?

src/test_c2s_srv.c, line 116 [r1] (raw file): Deleted

src/test_c2s_srv.c, line 273 [r1] (raw file): Every time I try a refactoring like that on this codebase, it ends up taking unreasonably long. It's not that I like it the way it is, but changes like the one you named have proven in the past to be much more work than you imagine, and push it past the point where the expected effort is worth it. Changing the variable names has already been a big help, and hopefully increases readability to the point where we can feel comfortable leaving sleeping dogs lie.

src/test_c2s_srv.c, line 297 [r1] (raw file): Swapped the order of the two blocks.

src/test_c2s_srv.c, line 324 [r1] (raw file): Now returns -EIO

src/test_c2s_srv.c, line 330 [r1] (raw file): Changed the condition to mirror the actually desired condition - if this was the last loop and we didn't finish accepting all connections, then exit.

src/test_c2s_srv.c, line 343 [r1] (raw file): Done.

src/test_c2s_srv.c, line 455 [r1] (raw file): Right you are! I recreated a bug from the code this descended from, and I even reported it as a bug a while back. Fixed.

src/test_c2s_srv.c, line 493 [r1] (raw file): The problem is it has to return many things. I'll create it and see.

Results: Net increase in file size, but net decrease in SLOCCOUNT. Let's keep it.

src/test_s2c_srv.c, line 296 [r1] (raw file): Changed to better variable names, but I contend it does match the pattern of a for loop, just not a foreach loop.

There is a starting condition, an end condition, and an increment that gets performed each round.

src/test_s2c_srv.c, line 328 [r1] (raw file): Nope. This is the confusing-as-hell control flow I inherited. If the loop should go around again, continue. Otherwise, log that you have connected and break.

I have refactored the control flow to something sensible.

src/test_s2c_srv.c, line 333 [r1] (raw file): Done.

src/test_s2c_srv.c, line 562 [r1] (raw file): I made a raw_write() function.

src/test_s2c_srv.c, line 583 [r1] (raw file): Urgh. A copy&paste gone wrong in a dead code path. Very nice catch.

src/test_s2c_srv.c, line 806 [r1] (raw file): It was redundant in the light of the earlier "shutdown_connection".

src/web100srv.c, line 164 [r1] (raw file): Sorry, got cut off.

...cover up bad network performance and make things look like they are healthy when in fact they are the opposite of healthy. In particular, it is quite possible to reason "1 flow got speed X, so three flows should be able to get at least X/3 each" but the reverse of "3 flows got speeds X, Y, and Z, so one flow should be able to get X+Y+Z" is not true. In particular, multiflow measurement's only advantage over single flow measurement is that it hides the effects of packet loss.

src/web100srv.c, line 207 [r1] (raw file): Changed back to the old order

src/web100srv.c, line 221 [r1] (raw file): Fixed.

src/web100srv.c, line 1467 [r1] (raw file): Not my code, but I'll make that change because it seems so obviously sensible.

src/web100srv.c, line 1469 [r1] (raw file): I didn't write this, so I'd prefer not to change it more than I already have with clang-format.

src/web100srv.c, line 1469 [r1] (raw file): Not me. This is "moved" code, not "written by Peter code".

src/web100srv.c, line 1471 [r1] (raw file): Not my code, but changed to a strncpy.

src/web100srv.c, line 2000 [r1] (raw file): They do, but I wanted the code to document that I knew those fields existed and they should be affirmatively set to zero.

src/web100srv_unit_tests.c, line 15 [r1] (raw file): Done.

src/web100srv_unit_tests.c, line 17 [r1] (raw file): It is a great help when debugging (can add -ddddddd to get full debug info from the server) and is also to start the server with TLS support. Adding that flexibility allowed us to eliminate a lot of redundant code around starting the server with multiport or starting the server with tls.

src/web100srv_unit_tests.c, line 17 [r1] (raw file): Done, albeit with different names.

src/web100srv_unit_tests.c, line 19 [r1] (raw file): Moved the declaration down to just above the while loop so that the comment is clearer

src/web100srv_unit_tests.c, line 41 [r1] (raw file): Nope, the server port. The server pid is available immediately, but the server port is only available after the server has successfully started. Basically, this is meant to be a way of losing the race condition where the test begins but the server's startup routines are not yet complete.

src/web100srv_unit_tests.c, line 45 [r1] (raw file): I don't like that because server_status is an outparam for waitid, and while I am pretty sure that things will resolve correctly, I'm not 100% sure.

src/web100srv_unit_tests.c, line 52 [r1] (raw file): I have changed the variable names. The basic issue is that system() wants a char* but exec() wants a char**

Hiding that difference seems like it wouldn't be worth it.

src/web100srv_unit_tests.c, line 75 [r1] (raw file): We did keep the non-multiport test. It's test_e2e. Comments have been added to that effect.

src/web100srv_unit_tests.c, line 134 [r1] (raw file): log_print is the right thing. Changed.

src/web100srv_unit_tests.c, line 134 [r1] (raw file): Both are okay says the OED, although cannot is more frequent.

src/web100srv_unit_tests.c, line 208 [r1] (raw file): Noted. This was another one like web100srv.c where it needed to be merged by hand instead of by full accept & copy, but I only caught that need in web100srv.c and not web100srv_unit_tests.c.

src/web100srv_unit_tests.c, line 286 [r1] (raw file): You are correct. That should have been excised. It is now.

src/web100srv_unit_tests.c, line 310 [r1] (raw file): It may take a lot less time to fail :)

I wanted to emphasize that taking a long time was an expected condition when the test is working and going to pass, so I wrote the English that way on purpose. If you find it jarring, I will change it.

src/test_c2s_clt.c, line 133 [r1] (raw file): Done.

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

Review status: 14 of 18 files reviewed at latest revision, 21 unresolved discussions.

src/test_c2s_srv.c, line 273 [r1] (raw file): Letting it go because of the time crunch, but I'd normally insist on not adding code to make the unmaintainable function worse.

I'd still like at least a split into two loops. Combining multiple indices into one for loop is confusing, especially since we only increment retries

src/test_c2s_srv.c, line 27 [r2] (raw file): Can we clarify that buff is essentially just temp, caller-provided storage and this function is not meant to populate it in any meaningful way?

src/test_c2s_srv.c, line 55 [r2] (raw file): "mark it done" is kind of confusing because we're not actually marking/doing anything. Suggest "so continue to the next stream."

src/test_c2s_srv.c, line 72 [r2] (raw file): maxfd -> max_fd

src/test_c2s_srv.c, line 76 [r2] (raw file): I prefer the inline declaration of i but it seems inconsistent with the rest of the file. At the very least, we should do the same thing in connections_to_fd_set and close_all_connections

src/test_s2c_srv.c, line 296 [r1] (raw file): That's true of retries, but not stream. The stream iteration matches a while

src/test_s2c_srv.c, line 806 [r1] (raw file): Can we update the comment?

src/web100srv.c, line 1471 [r1] (raw file): We still need the size check: sizeof(meta.web_variables_log) >= (strlen(dir) + 1) otherwise the strncpy can overflow the buffer (I think the buffer sizes currently prevent this, but those can change).

src/web100srv_unit_tests.c, line 41 [r1] (raw file): I think the comment is confusing because it makes me think we're trying to retrieve the server port number, but we're not. Can you integrate your code review comment into the code comment?

src/web100srv_unit_tests.c, line 45 [r1] (raw file): Is the concern that you're not sure && guarantees left to right evaluations? It does:

(4). Unlike the bitwise binary & operator, the && operator guarantees left-to-right evaluation; there is a sequence point after the evaluation of the first operand. If the first operand compares equal to 0, the second operand is not evaluated.

http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf

That said, I don't want to rely on things that are not well known because then it's harder for other developers to recognize that it's correct. I thought this was well known but if it's something you don't consider well known, I'll let this go.

src/web100srv_unit_tests.c, line 25 [r2] (raw file): I think this comment would make more sense immediately before the while loop. As written, it seems like index 0 is supposed to be NULL.

src/web100srv_unit_tests.c, line 26 [r2] (raw file): Trailing whitespace.

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

Reviewed 5 of 5 files at r2. Review status: all files reviewed at latest revision, 23 unresolved discussions.

src/test_c2s_srv.c, line 330 [r1] (raw file): I don't understand. Doesn't this cause us to log success at log level 6? The error message seems to imply an error.

src/test_c2s_srv.c, line 173 [r2] (raw file): maxfd->max_fd

(weird Reviewable bug isn't letting me comment on the right line)

src/test_c2s_srv.c, line 375 [r2] (raw file): (no action required) Doesn't this mean the retries portion of the for loop conditional will always be true? Smells more like a while loop to me...

src/test_c2s_srv.c, line 499 [r2] (raw file): Suggest rewriting to get rid of the redundant msgretvalue condition:

if (msgretvalue == -1) {
  if (errno == EINTR)) {
      // select interrupted. Continue waiting for activity on socket
      continue;
    } else {
      log_println(1, "Error while trying to wait for incoming data in c2s: %s",
                  strerror(errno));
      break;
    }
}

src/web100srv_unit_tests.c, line 75 [r1] (raw file): Ah, gotcha. I thought ext was for tests that used MSG_EXTENDED_LOGIN

Comments from the review on Reviewable.io

pboothe commented 9 years ago

Review status: all files reviewed at latest revision, 8 unresolved discussions.

src/test_c2s_srv.c, line 273 [r1] (raw file): moved conn_index outside and renamed retries to attempts, as discussed offline

src/test_c2s_srv.c, line 330 [r1] (raw file): It does. There used to be a lot more break and continue in the loop which skipped over this for errors. Moved to inside an error-handling condition.

src/test_c2s_srv.c, line 27 [r2] (raw file): Done.

src/test_c2s_srv.c, line 55 [r2] (raw file): Deleted the condition, as it didn't actually do anything at all.

src/test_c2s_srv.c, line 72 [r2] (raw file): Done (throughout the file).

src/test_c2s_srv.c, line 76 [r2] (raw file): Old instinct from too much C++. Changed to c-style.

src/test_c2s_srv.c, line 173 [r2] (raw file): Fixed.

src/test_c2s_srv.c, line 375 [r2] (raw file): Done. Discussed offline

src/test_c2s_srv.c, line 499 [r2] (raw file): Done, nice catch.

src/test_s2c_srv.c, line 296 [r1] (raw file): Performed the same change as we did for the other such loop.

src/test_s2c_srv.c, line 806 [r1] (raw file): Done.

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

src/web100srv_unit_tests.c, line 41 [r1] (raw file): Done.

src/web100srv_unit_tests.c, line 45 [r1] (raw file): It's pretty well known, but it looks confusing to my eye - I have to read the line a few times, while with the other I am confident that the order of operations is correct.

I have added the exit code to the test failure output, as more information is always better when a test fails.

src/web100srv_unit_tests.c, line 25 [r2] (raw file): Done.

src/web100srv_unit_tests.c, line 26 [r2] (raw file): Fixed all trailing whitespace.

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

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

src/test_s2c_srv.c, line 328 [r1] (raw file): Can you add a comment explaining this. The condition is when all streams have connected, right?

It seems like there's an assumption that if we exit the loop without returning (i.e. get to 370) that we connected successfully, but there if we continue enough to exhaust the retries, then we exit without returning RETRY_EXCEEDED_WAITING_CONNECT.

Does it make sense to move these snippets to after the loop:

 if (stream == streamsNum) {
   protolog_procstatus(testOptions->child0, testids, CONNECT_TYPE,
                       PROCESS_STARTED, xmitsfd[0].socket);
 }
if ((stream == streamsNum) && (attempts == (RETRY_COUNT*streamsNum - 1)) {
        log_println(
            6,
            "s2c child %d, unable to open connection, return from test",
            testOptions->child0);
        return RETRY_EXCEEDED_WAITING_CONNECT;  // retry exceeded. exit
      }

Possible I'm misunderstanding the loop.

src/test_s2c_srv.c, line 806 [r1] (raw file): It doesn't look like we're still closing file descriptors here. Are we?

src/web100srv.c, line 1474 [r3] (raw file): tab

src/web100srv_unit_tests.c, line 43 [r3] (raw file): immediately

Comments from the review on Reviewable.io

pboothe commented 9 years ago

Review status: 15 of 18 files reviewed at latest revision, 7 unresolved discussions.

src/test_s2c_srv.c, line 328 [r1] (raw file): Refactored it out to after the loop and it became a lot cleaner. Good catch!

src/test_s2c_srv.c, line 806 [r1] (raw file): Changed the comment to be generic enough to make sure that the comment and code will remain in sync as long as the spirit of the comment is maintained.

src/web100srv.c, line 1474 [r3] (raw file): :embarrassed face emoji:

src/web100srv_unit_tests.c, line 43 [r3] (raw file): :embaraassed face emoji:

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

Review status: 15 of 18 files reviewed at latest revision, 9 unresolved discussions.

src/test_c2s_srv.c, line 329 [r4] (raw file): This seems like a weird condition. It seems somewhat redundant to the loop condition. Why even let the loop go to the next iteration if we're just going to cut it off if it succeeds?

I think this might go away once we check for excessive retries after the loop.

src/test_c2s_srv.c, line 372 [r4] (raw file): This needs the same fix as in the other loop, no?

Comments from the review on Reviewable.io

pboothe commented 9 years ago

Review status: 15 of 18 files reviewed at latest revision, 7 unresolved discussions.

src/test_c2s_srv.c, line 329 [r4] (raw file): It did indeed.

src/test_c2s_srv.c, line 372 [r4] (raw file): Done.

Comments from the review on Reviewable.io

mtlynch commented 9 years ago

LGTM

Review status: 14 of 18 files reviewed at latest revision, 7 unresolved discussions.

Comments from the review on Reviewable.io