Skip to content

Commit

Permalink
S2c freeze fix (#57)
Browse files Browse the repository at this point in the history
* Three small bugfixes, one of which was causing real issues for MLab servers in developing nations.
A one-line compiler warning fix for test_c2s.
Fixing watchdog timer issues in web100srv.  (The server bug)
Making end-to-end unit tests less flaky by no longer just assuming that every port in the range [1024, 31024) is fine for the server to take as its own.

* Moved helper functions to testutils, so that they can be used by both c2s
and s2c. Used those new helper functions to fix a freezing error in s2c,
where the s2c test pauses until it hears back from the packet trace
engine, and so if the packet trace engine fails to start, then the s2c
test just freezes forever.

* Updated in response to comments by Josh to eliminate `strlen()`.
  • Loading branch information
pboothe committed Jun 8, 2016
1 parent f509bb0 commit 225d009
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 64 deletions.
14 changes: 7 additions & 7 deletions src/Makefile.am
Expand Up @@ -108,7 +108,7 @@ endif
web100srv_SOURCES = web100srv.c web100-util.c web100-pcap.c web100-admin.c runningtest.c \
network.c usage.c utils.c mrange.c logging.c testoptions.c ndtptestconstants.c \
protocol.c test_sfw_srv.c test_meta_srv.c ndt_odbc.c strlutils.c heuristics.c \
test_c2s_srv.c test_s2c_srv.c test_mid_srv.c jsonutils.c websocket.c
test_c2s_srv.c test_s2c_srv.c test_mid_srv.c testutils.c jsonutils.c websocket.c
web100srv_LDFLAGS = $(NDTLDFLAGS) $(I2UTILLDFLAGS)
web100srv_LDADD = $(NDTLIBS) $(I2UTILLIBS) $(I2UTILLIBDEPS) -lpthread $(ZLIB) $(JSONLIB) $(OPENSSL_LDFLAGS) $(OPENSSL_LIBS)
web100srv_CPPFLAGS ='-DBASEDIR="$(ndtdir)"' -DFORCE_WEB100 $(OPENSSL_INCLUDES)
Expand All @@ -130,8 +130,8 @@ web100_websocket_unit_tests_DEPENDENCIES = $(I2UTILLIBDEPS)
web100_testoptions_unit_tests_SOURCES = testoptions_unit_tests.c testoptions.c unit_testing.c \
heuristics.c jsonutils.c logging.c mrange.c ndt_odbc.c ndtptestconstants.c \
network.c protocol.c runningtest.c strlutils.c test_c2s_srv.c test_meta_srv.c \
test_mid_srv.c test_s2c_srv.c test_sfw_srv.c utils.c web100-pcap.c web100-util.c \
web100srv.c websocket.c
test_mid_srv.c test_s2c_srv.c test_sfw_srv.c testutils.c utils.c web100-pcap.c \
web100-util.c web100srv.c websocket.c
web100_testoptions_unit_tests_LDFLAGS = $(NDTLDFLAGS) $(I2UTILLDFLAGS)
web100_testoptions_unit_tests_LDADD = $(NDTLIBS) $(I2UTILLIBS) $(I2UTILLIBDEPS) -lpthread $(ZLIB) $(JSONLIB) $(OPENSSL_LDFLAGS) $(OPENSSL_LIBS)
web100_testoptions_unit_tests_CPPFLAGS ='-DBASEDIR="$(ndtdir)"' -DFORCE_WEB100 -DUSE_WEB100SRV_ONLY_AS_LIBRARY -Wall -Wno-unused-variable -Wno-unused-function $(OPENSSL_INCLUDES)
Expand All @@ -140,7 +140,7 @@ web100_testoptions_unit_tests_DEPENDENCIES = $(I2UTILLIBDEPS)
web10gsrv_SOURCES = web100srv.c web100-util.c web100-pcap.c web100-admin.c runningtest.c \
network.c usage.c utils.c mrange.c logging.c testoptions.c ndtptestconstants.c \
protocol.c test_sfw_srv.c test_meta_srv.c ndt_odbc.c strlutils.c heuristics.c \
test_c2s_srv.c test_s2c_srv.c test_mid_srv.c web10g-util.c jsonutils.c websocket.c
test_c2s_srv.c test_s2c_srv.c test_mid_srv.c testutils.c web10g-util.c jsonutils.c websocket.c
web10gsrv_LDFLAGS = $(NDTLDFLAGS) $(I2UTILLDFLAGS)
web10gsrv_LDADD = $(NDTLIBS) $(I2UTILLIBS) $(I2UTILLIBDEPS) -lpthread $(ZLIB) $(JSONLIB) $(OPENSSL_LDFLAGS) $(OPENSSL_LIBS)
web10gsrv_CPPFLAGS = '-DBASEDIR="$(ndtdir)"' $(OPENSSL_INCLUDES)
Expand All @@ -156,8 +156,8 @@ web10g_websocket_unit_tests_DEPENDENCIES = $(I2UTILLIBDEPS)
web10g_testoptions_unit_tests_SOURCES = testoptions_unit_tests.c testoptions.c unit_testing.c \
heuristics.c jsonutils.c logging.c mrange.c ndt_odbc.c ndtptestconstants.c \
network.c protocol.c runningtest.c strlutils.c test_c2s_srv.c test_meta_srv.c \
test_mid_srv.c test_s2c_srv.c test_sfw_srv.c utils.c web100-pcap.c web100-util.c \
web100srv.c web10g-util.c websocket.c usage.c web100-admin.c
test_mid_srv.c test_s2c_srv.c test_sfw_srv.c testutils.c utils.c web100-pcap.c \
web100-util.c web100srv.c web10g-util.c websocket.c usage.c web100-admin.c
web10g_testoptions_unit_tests_LDFLAGS = $(NDTLDFLAGS) $(I2UTILLDFLAGS)
web10g_testoptions_unit_tests_LDADD = $(NDTLIBS) $(I2UTILLIBS) $(I2UTILLIBDEPS) -lpthread $(ZLIB) $(JSONLIB) $(OPENSSL_LDFLAGS) $(OPENSSL_LIBS)
web10g_testoptions_unit_tests_CPPFLAGS ='-DBASEDIR="$(ndtdir)"' -DFORCE_WEB10G -DUSE_WEB100SRV_ONLY_AS_LIBRARY -Wall -Wno-unused-variable -Wno-unused-function $(OPENSSL_INCLUDES)
Expand Down Expand Up @@ -189,5 +189,5 @@ $(I2UTILLIBDEPS):

EXTRA_DIST = clt_tests.h logging.h mrange.h network.h protocol.h testoptions.h test_sfw.h test_meta.h \
troute.h tr-tree.h usage.h utils.h varinfo.h web100-admin.h web100srv.h ndt_odbc.h runningtest.h ndtptestconstants.h \
heuristics.h strlutils.h test_results_clt.h tests_srv.h jsonutils.h unit_testing.h websocket.h third_party/safe_iop.h
heuristics.h strlutils.h test_results_clt.h tests_srv.h testutils.h jsonutils.h unit_testing.h websocket.h third_party/safe_iop.h

44 changes: 1 addition & 43 deletions src/test_c2s_srv.c
Expand Up @@ -15,6 +15,7 @@
#include "ndtptestconstants.h"
#include "utils.h"
#include "testoptions.h"
#include "testutils.h"
#include "runningtest.h"
#include "logging.h"
#include "protocol.h"
Expand Down Expand Up @@ -180,49 +181,6 @@ void drain_old_clients(Connection* c2s_conns, int streamsNum, char* buff, size_t
}
}

/** Makes the passed-in file descriptor into one that will not block.
* @param fd the file descriptor
* @returns non-zero if successful, zero on failure with errno as set by fcntl.
*/
int make_non_blocking(int fd) {
int flags;
flags = fcntl(fd, F_GETFL, NULL);
if (flags == -1) return 0;
return fcntl(fd, F_SETFL, flags | O_NONBLOCK) == 0;
}

/** Attempts to use the pipe to shutdown packet tracing. Will not block. After
* this function the file descriptors of the pipe are likely set to
* non-blocking. This should not affect any code because any shutdown messages
* sent should be the last usage of the pipe anyway.
* @param mon_pipe the pipe on which to send shutdown messages.
*/
void packet_trace_emergency_shutdown(int *mon_pipe) {
// Attempt to shut down the trace, but only after making sure that all
// attempts to write to the pipe will never block.
if (make_non_blocking(mon_pipe[1]) && make_non_blocking(mon_pipe[0])) {
stop_packet_trace(mon_pipe);
} else {
log_println(0,
"Couldn't make pipe non-blocking (errno=%d) and so was "
"unable to safely call stop_packet_trace",
errno);
}
}

/** Waits up to one second for the passed-in fd to become readable.
* @param fd the file descriptor to wait for
* @returns true if the fd is readable, false otherwise
*/
int wait_for_readable_fd(int fd) {
fd_set rfd;
struct timeval sel_tv = {0};
FD_ZERO(&rfd);
FD_SET(fd, &rfd);
sel_tv.tv_sec = 1; // Wait for up to 1 second
return (1 == select(fd + 1, &rfd, NULL, NULL, &sel_tv));
}

// How long to sleep to avoid a race condition. This is a bad hack.
// At 150k tests per day, this one sleep(2) wastes 83 hours of peoples'
// lives every day.
Expand Down
46 changes: 32 additions & 14 deletions src/test_s2c_srv.c
Expand Up @@ -16,6 +16,7 @@
#include "ndtptestconstants.h"
#include "utils.h"
#include "testoptions.h"
#include "testutils.h"
#include "runningtest.h"
#include "logging.h"
#include "protocol.h"
Expand Down Expand Up @@ -125,6 +126,7 @@ int test_s2c(Connection *ctl, tcp_stat_agent *agent, TestOptions *testOptions,
pid_t s2c_childpid = 0; // s2c_childpid

char tmpstr[256]; // string array used for temp storage of many char*
size_t tmpstr_len = 0;
struct sockaddr_storage cli_addr[MAX_STREAMS];
struct throughputSnapshot *lastThroughputSnapshot;

Expand Down Expand Up @@ -159,6 +161,8 @@ int test_s2c(Connection *ctl, tcp_stat_agent *agent, TestOptions *testOptions,
enum TEST_ID testids = extended ? S2C_EXT : S2C;
char snaplogsuffix[256] = "s2c_snaplog";

int packet_trace_running = 0;

memset(xmitsfd, 0, sizeof(xmitsfd));
for (i = 0; i < MAX_STREAMS; i++) {
streams[i].snapArgs.snap = NULL;
Expand Down Expand Up @@ -393,19 +397,33 @@ int test_s2c(Connection *ctl, tcp_stat_agent *agent, TestOptions *testOptions,
log_println(0, "S2C test error: can't create child process.");
}
}
memset(tmpstr, 0, 256);
for (i = 0; i < 5; i++) { // read nettrace file name into "tmpstr"
ret = read(mon_pipe[0], tmpstr, 128);
// socket interrupted, try reading again
if ((ret == -1) && (errno == EINTR))
continue;
break;
}

if (strlen(tmpstr) > 5)
memcpy(meta.s2c_ndttrace, tmpstr, strlen(tmpstr));
// name of nettrace file passed back from pcap child copied into meta
// structure
packet_trace_running = wait_for_readable_fd(mon_pipe[0]);
if (packet_trace_running) {
memset(tmpstr, 0, 256);
tmpstr_len = 0;
for (i = 0; i < 5; i++) { // read nettrace file name into "tmpstr"
ret = read(mon_pipe[0], tmpstr, 128);
// socket interrupted, try reading again
if ((ret == -1) && (errno == EINTR)) {
continue;
} else if (ret == 128) {
tmpstr[127] = '\0'; // ensure the string is well-terminated.
tmpstr_len = 128;
} else if (ret > 0) {
tmpstr_len = ret;
}
break;
}

if (tmpstr_len > 5)
memcpy(meta.s2c_ndttrace, tmpstr, tmpstr_len);
// name of nettrace file passed back from pcap child copied into meta
// structure
} else {
log_println(0, "Packet trace was unable to be created");
packet_trace_emergency_shutdown(mon_pipe);
}
}

/* experimental code, delete when finished */
Expand Down Expand Up @@ -645,7 +663,7 @@ int test_s2c(Connection *ctl, tcp_stat_agent *agent, TestOptions *testOptions,
* Skip this step if speed-chk isn't running.
*/

if (getuid() == 0) {
if (packet_trace_running) {
log_println(1, "Signal USR2(%d) sent to child [%d]", SIGUSR2,
s2c_childpid);
testOptions->child2 = s2c_childpid;
Expand Down Expand Up @@ -807,7 +825,7 @@ int test_s2c(Connection *ctl, tcp_stat_agent *agent, TestOptions *testOptions,
"S2C test - failed to send finalize message to pid=%d",
s2c_childpid);

if (getuid() == 0) {
if (packet_trace_running) {
stop_packet_trace(mon_pipe);
}

Expand Down
48 changes: 48 additions & 0 deletions src/testutils.c
@@ -0,0 +1,48 @@
#include <fcntl.h>
#include <unistd.h>
#include "logging.h"
#include "testoptions.h"
#include "testutils.h"

/** Makes the passed-in file descriptor into one that will not block.
* @param fd the file descriptor
* @returns non-zero if successful, zero on failure with errno as set by fcntl.
*/
int make_non_blocking(int fd) {
int flags;
flags = fcntl(fd, F_GETFL, NULL);
if (flags == -1) return 0;
return fcntl(fd, F_SETFL, flags | O_NONBLOCK) == 0;
}

/** Attempts to use the pipe to shutdown packet tracing. Will not block. After
* this function the file descriptors of the pipe are likely set to
* non-blocking. This should not affect any code because any shutdown messages
* sent should be the last usage of the pipe anyway.
* @param mon_pipe the pipe on which to send shutdown messages.
*/
void packet_trace_emergency_shutdown(int *mon_pipe) {
// Attempt to shut down the trace, but only after making sure that all
// attempts to write to the pipe will never block.
if (make_non_blocking(mon_pipe[1]) && make_non_blocking(mon_pipe[0])) {
stop_packet_trace(mon_pipe);
} else {
log_println(0,
"Couldn't make pipe non-blocking (errno=%d) and so was "
"unable to safely call stop_packet_trace",
errno);
}
}

/** Waits up to one second for the passed-in fd to become readable.
* @param fd the file descriptor to wait for
* @returns true if the fd is readable, false otherwise
*/
int wait_for_readable_fd(int fd) {
fd_set rfd;
struct timeval sel_tv = {0};
FD_ZERO(&rfd);
FD_SET(fd, &rfd);
sel_tv.tv_sec = 1; // Wait for up to 1 second
return (1 == select(fd + 1, &rfd, NULL, NULL, &sel_tv));
}
5 changes: 5 additions & 0 deletions src/testutils.h
@@ -0,0 +1,5 @@
/* These are helper methods which are used in multiple test_XXX_srv.c files. */

int make_non_blocking(int fd);
int wait_for_readable_fd(int fd);
void packet_trace_emergency_shutdown(int *mon_pipe);

0 comments on commit 225d009

Please sign in to comment.