From 91e152de27334562a9f09b194aa1126b7cf33e01 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 7 Jan 2017 14:30:00 +0100 Subject: [PATCH 01/10] Don't assume server supports all test types The NDT client assumes that the server supports all test types and the official NDT server respects this assumption. However, the [neubot/botticelli](https://github.com/neubot/botticelli) server only implements TEST_META, TEST_S2C, and TEST_S2C. Therefore, when using the NDT client with a botticelli server, the client crahes in processing the results of the TEST_MID that however has not been executed. Fix by making sure that, when processing results, we use a bitmak where only bits corresponding to tests that run are actually set. (There is also another reason why NDT client crashes when testing with botticelli, addressed by neubot/botticelli#18.) Problems between NDT client and botticelli reported by @nkinkade, thanks! --- src/web100clt.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/web100clt.c b/src/web100clt.c index 191ab1f..f704458 100644 --- a/src/web100clt.c +++ b/src/web100clt.c @@ -924,6 +924,12 @@ int main(int argc, char *argv[]) { } else ptr = strtok_r(buff, " ", &strtokbuf); + /* + * Server may support less test types that we support. Hence, collect + * into this var _only_ the test types that were run successfully. + */ + unsigned char run_tests = 0; + // Run all tests requested, based on the ID. while (ptr) { if (check_int(ptr, &testId)) { @@ -935,39 +941,45 @@ int main(int argc, char *argv[]) { if (test_mid_clt(ctlSocket, tests, host, conn_options, buf_size, mid_resultstr, jsonSupport)) { log_println(0, "Middlebox test FAILED!"); - tests &= (~TEST_MID); + } else { + run_tests |= TEST_MID; } break; case TEST_C2S: if (test_c2s_clt(ctlSocket, tests, host, conn_options, buf_size, &c2s_ThroughputSnapshots, jsonSupport, 0)) { log_println(0, "C2S throughput test FAILED!"); - tests &= (~TEST_C2S); + } else { + run_tests |= TEST_C2S; } break; case TEST_C2S_EXT: if (test_c2s_clt(ctlSocket, tests, host, conn_options, buf_size, &c2s_ThroughputSnapshots, jsonSupport, 1)) { log_println(0, "Extended S2C throughput test FAILED!"); - tests &= (~TEST_C2S_EXT); + } else { + run_tests |= TEST_C2S_EXT; } break; case TEST_S2C: if (test_s2c_clt(ctlSocket, tests, host, conn_options, buf_size, resultstr, &s2c_ThroughputSnapshots, jsonSupport, 0)) { log_println(0, "S2C throughput test FAILED!"); - tests &= (~TEST_S2C); + } else { + run_tests |= TEST_S2C; } break; case TEST_S2C_EXT: if (test_s2c_clt(ctlSocket, tests, host, conn_options, buf_size, resultstr, &s2c_ThroughputSnapshots, jsonSupport, 1)) { log_println(0, "Extended S2C throughput test FAILED!"); - tests &= (~TEST_S2C_EXT); + } else { + run_tests |= TEST_S2C_EXT; } break; case TEST_SFW: if (test_sfw_clt(ctlSocket, tests, host, conn_options, jsonSupport)) { log_println(0, "Simple firewall test FAILED!"); - tests &= (~TEST_SFW); + } else { + run_tests |= TEST_SFW; } break; case TEST_META: @@ -977,7 +989,8 @@ int main(int argc, char *argv[]) { if (test_meta_clt(ctlSocket, tests, host, conn_options, client_app_id, jsonSupport)) { log_println(0, "META test FAILED!"); - tests &= (~TEST_META); + } else { + run_tests |= TEST_META; } break; default: @@ -987,6 +1000,9 @@ int main(int argc, char *argv[]) { ptr = strtok_r(NULL, " ", &strtokbuf); } + /* Make sure we only process tests that were run */ + tests = run_tests; + /* get the final results from server. * * The results are encapsulated by the MSG_RESULTS messages. The last From 7137663e03c2b643f6e12da31c327cc7fd532a80 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 7 Jan 2017 18:28:51 +0100 Subject: [PATCH 02/10] web100clt.c: make results parsing more robust 1) Do not assume that the first line we receive contains a space 2) Do not assume that the first line we receive contains an integer 3) Be robust to the case where input is an empty string 4) Do not assume that after the first token delimited by space we will find a second token delimited by newline Tested under the following conditions: a) web100clt -n ndt.iupui.mlab1.trn01.measurement-lab.org that sends back all variables and checked via printf() that the variables that are parsed by the new code are the ones received in resultstr b) web100clt -n neubot.mlab.mlab1.mil01.measurement-lab.org that at the moment is running botticelli v0.0.5 (which is buggy and doesn't send any MSG_RESULTS messages) and make sure it does not crash c) web100clt -n neubot.mlab.mlab1.trn01.measurement-lab.org that at the moment is running botticelli v0.0.6 (which sends a single dummy variable not considered by NDT) and make sure it doesn't crash Note that a) and c) did not changed after this patch. What this patch changes is the behavior in case b). xref: neubot/botticelli#18. --- src/web100clt.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/web100clt.c b/src/web100clt.c index 191ab1f..b2ecd70 100644 --- a/src/web100clt.c +++ b/src/web100clt.c @@ -149,15 +149,12 @@ void testResults(char tests, char *testresult_str, char* host) { } sysvar = strtok(testresult_str, " "); - sysval = strtok(NULL, "\n"); - i = atoi(sysval); - save_int_values(sysvar, i); - for (;;) { - sysvar = strtok(NULL, " "); if (sysvar == NULL) break; sysval = strtok(NULL, "\n"); + if (sysval == NULL) + break; if (strchr(sysval, '.') == NULL) { i = atoi(sysval); save_int_values(sysvar, i); @@ -167,6 +164,7 @@ void testResults(char tests, char *testresult_str, char* host) { save_dbl_values(sysvar, &j); log_println(7, "Stored %0.2f (%s) in %s", j, sysval, sysvar); } + sysvar = strtok(NULL, " "); } // CountRTT = 615596; From 593821b472d9d91065339add8c31596fc3056d1a Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Thu, 9 Feb 2017 18:14:34 -0500 Subject: [PATCH 03/10] Add travis config to build with measurementlab/ndt-builder --- .travis.yml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..f318a34 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,30 @@ +# Build NDT and run unit tests. +# * Uses builder image from docker.io/measurementlab/ndt-builder. +# * Mounts ~/ndt as the build directory. +# * Executes unit tests within travis after build. (In development) + +dist: trusty +language: ruby + +services: + - docker + +before_install: + +cache: + directories: + +before_cache: + +before_script: + +script: +# Build and run tests. + - docker pull measurementlab/ndt-builder:latest + - cd ${TRAVIS_BUILD_DIR}; + - mkdir ndt + - docker run -v ${TRAVIS_BUILD_DIR}/ndt:/root/builder measurementlab/ndt-builder + - ls -l ${TRAVIS_BUILD_DIR}/ndt + +# TODO - execute unit tests that do not require web100. +# TODO - collect coverage stats and export to coveralls. From 33596ce9aa6aa833b94ac91ec098d580b048d812 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 15 Feb 2017 15:48:21 -0500 Subject: [PATCH 04/10] Fetch docker image from mlab-pub --- .travis.yml | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/.travis.yml b/.travis.yml index f318a34..5a875ad 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,30 +1,15 @@ # Build NDT and run unit tests. -# * Uses builder image from docker.io/measurementlab/ndt-builder. -# * Mounts ~/ndt as the build directory. -# * Executes unit tests within travis after build. (In development) +# * Uses docker build-tools container +# * Executes unit tests within travis after build. dist: trusty language: ruby - services: - - docker - -before_install: - -cache: - directories: - -before_cache: - -before_script: +- docker script: -# Build and run tests. - - docker pull measurementlab/ndt-builder:latest - - cd ${TRAVIS_BUILD_DIR}; - - mkdir ndt - - docker run -v ${TRAVIS_BUILD_DIR}/ndt:/root/builder measurementlab/ndt-builder - - ls -l ${TRAVIS_BUILD_DIR}/ndt +- docker pull gcr.io/mlab-pub/github-m-lab-builder:latest +# Run basic unit tests that don't require web100 +- docker run gcr.io/mlab-pub/github-m-lab-builder /root/ndt_build_and_test.sh -# TODO - execute unit tests that do not require web100. -# TODO - collect coverage stats and export to coveralls. +# TODO - collect coverage stats and export to coveralls. \ No newline at end of file From d47253a7b26560969e1d7027e9dfbd4e544b3f75 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Thu, 8 Dec 2016 10:41:09 -0500 Subject: [PATCH 05/10] Add log_first_n, cleanup --- src/logging.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/logging.h b/src/logging.h index 5846fdd..8f7b25c 100644 --- a/src/logging.h +++ b/src/logging.h @@ -32,7 +32,7 @@ char* get_logfile(); I2ErrHandle get_errhandle(); -// TODO: audit all the calls to log_print and log_println to maxmise ease of +// TODO: audit all the calls to log_print and log_println to maximise ease of // grepping through logs and eliminate the possibility of loglines that // don't start with the timestamp/pid/level/file/line prefix. #define log_print log_println @@ -40,6 +40,14 @@ void log_println_impl(int lvl, const char* file, int line, const char* format, . #define log_println(lvl, ...) \ log_println_impl((lvl), __FILE__, __LINE__, __VA_ARGS__) +#define log_first_n(lvl, n, ...) { \ + static int LOG_COUNT&&__LINE__ = n; \ + if (LOG_COUNT&&__LINE__ > 0) { \ + LOG_COUNT&&__LINE__--; \ + log_println_impl((lvl), __FILE__, __LINE__, __VA_ARGS__); \ + }} + + void log_free(void); void set_timestamp(); time_t get_timestamp(); From c7858fad6bc4f84b008ad1781d72efb53b8e96cc Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Mon, 6 Feb 2017 12:01:24 -0500 Subject: [PATCH 06/10] Add missing #include and make some functions static. --- src/genplot.c | 1 + src/test_s2c_srv.c | 2 ++ src/test_sfw_srv.c | 1 + src/testoptions.c | 1 + src/web100-util.c | 17 +++++++++-------- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/genplot.c b/src/genplot.c index 4ac65ba..419ccb7 100644 --- a/src/genplot.c +++ b/src/genplot.c @@ -27,6 +27,7 @@ #include #include #include +#include #include "web100srv.h" #include "usage.h" diff --git a/src/test_s2c_srv.c b/src/test_s2c_srv.c index 4eaecd8..564a464 100644 --- a/src/test_s2c_srv.c +++ b/src/test_s2c_srv.c @@ -11,6 +11,8 @@ #include #include #include +#include + #include "tests_srv.h" #include "strlutils.h" #include "ndtptestconstants.h" diff --git a/src/test_sfw_srv.c b/src/test_sfw_srv.c index 63d0cb7..63eabd3 100644 --- a/src/test_sfw_srv.c +++ b/src/test_sfw_srv.c @@ -8,6 +8,7 @@ #include #include +#include #include "test_sfw.h" #include "logging.h" diff --git a/src/testoptions.c b/src/testoptions.c index 08b5792..501af42 100644 --- a/src/testoptions.c +++ b/src/testoptions.c @@ -9,6 +9,7 @@ #include // #include #include +#include #include "testoptions.h" #include "network.h" diff --git a/src/web100-util.c b/src/web100-util.c index 2b0df05..6d778f8 100644 --- a/src/web100-util.c +++ b/src/web100-util.c @@ -9,6 +9,7 @@ #include #include #include +#include #include "logging.h" #include "network.h" @@ -821,7 +822,7 @@ int tcp_stat_get_data(tcp_stat_snap** snap, Connection* testsock, int streamsNum * * */ -int web100_rtt(int sock, web100_agent* agent, web100_connection* cn) { +static int web100_rtt(int sock, web100_agent* agent, web100_connection* cn) { web100_var* var; char buf[32]; web100_group* group; @@ -938,8 +939,8 @@ int tcp_stat_autotune(int sock, tcp_stat_agent* agent, tcp_stat_connection cn) { * 35 - cannot read value of RcvWinScale web100 variable. * */ -int tcp_stat_setbuff(int sock, tcp_stat_agent* agent, tcp_stat_connection cn, - int autotune) { +static int tcp_stat_setbuff(int sock, tcp_stat_agent* agent, tcp_stat_connection cn, + int autotune) { web100_var* var; char buf[32]; web100_group* group; @@ -1108,7 +1109,7 @@ void tcp_stat_logvars_to_file(char* webVarsValuesLog, int connNum, struct tcp_va fclose(file); } -tcp_stat_var agg_vars_sum(int connNum, int varId, struct tcp_vars* vars) { +static tcp_stat_var agg_vars_sum(int connNum, int varId, struct tcp_vars* vars) { int i; tcp_stat_var varValue = *&((tcp_stat_var *)&vars[0])[varId]; for (i = 1; i < connNum; ++i) { @@ -1117,7 +1118,7 @@ tcp_stat_var agg_vars_sum(int connNum, int varId, struct tcp_vars* vars) { return varValue; } -tcp_stat_var agg_vars_max(int connNum, int varId, struct tcp_vars* vars) { +static tcp_stat_var agg_vars_max(int connNum, int varId, struct tcp_vars* vars) { int i; tcp_stat_var varValue = *&((tcp_stat_var *)&vars[0])[varId]; for (i = 1; i < connNum; ++i) { @@ -1128,7 +1129,7 @@ tcp_stat_var agg_vars_max(int connNum, int varId, struct tcp_vars* vars) { return varValue; } -tcp_stat_var agg_vars_min(int connNum, int varId, struct tcp_vars* vars) { +static tcp_stat_var agg_vars_min(int connNum, int varId, struct tcp_vars* vars) { int i; tcp_stat_var varValue = *&((tcp_stat_var *)&vars[0])[varId]; for (i = 1; i < connNum; ++i) { @@ -1139,7 +1140,7 @@ tcp_stat_var agg_vars_min(int connNum, int varId, struct tcp_vars* vars) { return varValue; } -tcp_stat_var agg_vars_avg(int connNum, int varId, struct tcp_vars* vars) { +static tcp_stat_var agg_vars_avg(int connNum, int varId, struct tcp_vars* vars) { int i; tcp_stat_var varValue = *&((tcp_stat_var *)&vars[0])[varId]; for (i = 1; i < connNum; ++i) { @@ -1326,7 +1327,7 @@ int CwndDecrease(char* logname, u_int32_t *dec_cnt, * @param nwords integer length (in bytes) of the header * @return unsigned short checksum */ -unsigned short csum(unsigned short *buff, int nwords) { +static unsigned short csum(unsigned short *buff, int nwords) { /* generate a TCP/IP checksum for our packet */ register int sum = 0; From 2d321dda75564909997ddd7f48d537466b569c10 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 22 Mar 2017 14:27:27 -0400 Subject: [PATCH 07/10] remove confusing I2util discovery --- configure.ac | 71 ++++++++++------------------------------------------ 1 file changed, 13 insertions(+), 58 deletions(-) diff --git a/configure.ac b/configure.ac index 1932563..7cb1418 100644 --- a/configure.ac +++ b/configure.ac @@ -191,77 +191,32 @@ else fi +# +# configure I2util +# AC_ARG_WITH(I2util, AC_HELP_STRING([--with-I2util=], [defaults to building I2util under NDT if exists]), with_I2util=$withval, with_I2util=yes) -# -# find I2util -# I2UTILLDFLAGS="" I2UTILLIBS="" I2UTILLIBDEPS="" I2UTILLIBMAKE="" I2UTILINCS="" -if test -n "$with_I2util" && test "$with_I2util" != "yes"; then - I2util_dir=${with_I2util} - case $I2util_dir in - /*) ;; # already an absolute path - *) I2util_dir="`pwd`/$I2util_dir" ;; - esac - I2util_include_dir=`dirname $with_I2util`/include - I2UTILINCS="-I$I2util_include_dir $I2UTILINCS" - I2UTILLDFLAGS="-L$I2util_dir $I2UTILLDFLAGS" - I2UTILLIBDEPS="$I2util_dir/libI2util.a" -fi - -# Save the old CFLAGS and LDFLAGS -OLD_LDFLAGS="$LDFLAGS" -OLD_CFLAGS="$CFLAGS" - -CFLAGS="$CFLAGS $I2UTILINCS" -LDFLAGS="$CFLAGS $I2UTILLDFLAGS" - -AC_SEARCH_LIBS([I2AddrByNode],I2util, HAVE_I2UTIL_LIBS=1,HAVE_I2UTIL_LIBS=0) -AC_CHECK_HEADERS([I2util/util.h I2util/conf.h], HAVE_I2UTIL_HEADERS=1,HAVE_I2UTIL_HEADERS=0) - -LDFLAGS="$OLD_LDFLAGS" -CFLAGS="$OLD_CFLAGS" - -if test "$HAVE_I2UTIL_HEADERS" != "1" || test "$HAVE_I2UTIL_LIBS" != "1"; then - # now, check for sub-build/sub-configure - if test -d I2util/I2util; then - AC_CONFIG_SUBDIRS(I2util) - TOP_BUILD_DIRS="I2util $TOP_BUILD_DIRS" - I2util_dir='${top_srcdir}/I2util' - I2UTILINCS="-I$I2util_dir $I2UTILINCS" - I2UTILLDFLAGS="-L$I2util_dir/I2util $I2UTILLDFLAGS" - I2UTILLIBDEPS="$I2util_dir/I2util/libI2util.a" - I2UTILLIBMAKE="cd $I2util_dir; make" - else - AC_MSG_FAILURE([I2util required to build NDT]) - fi - - # Save the old CFLAGS and LDFLAGS - OLD_LDFLAGS="$LDFLAGS" - OLD_CFLAGS="$CFLAGS" - - CFLAGS="$CFLAGS $I2UTILINCS" - LDFLAGS="$CFLAGS $I2UTILLDFLAGS" - - AC_SEARCH_LIBS([I2AddrByNode],I2util, ,AC_MSG_ERROR([Couldn't find I2util library])) - AC_CHECK_HEADERS([I2util/util.h I2util/conf.h], ,AC_MSG_ERROR([Couldn't find I2util header files]), [AC_INCLUDES_DEFAULT]) - - LDFLAGS="$OLD_LDFLAGS" - CFLAGS="$OLD_CFLAGS" - - I2UTILLIBS="$I2UTILLDFLAGS -lI2util" +if test -d I2util/I2util; then + AC_CONFIG_SUBDIRS(I2util) + TOP_BUILD_DIRS="I2util $TOP_BUILD_DIRS" + I2util_dir='${top_srcdir}/I2util' + I2UTILINCS="-I$I2util_dir $I2UTILINCS" + I2UTILLDFLAGS="-L$I2util_dir/I2util $I2UTILLDFLAGS" + I2UTILLIBDEPS="$I2util_dir/I2util/libI2util.a" + I2UTILLIBMAKE="cd $I2util_dir; make" +else + AC_MSG_FAILURE([I2util required to build NDT]) fi -I2UTILLIBS="$I2UTILLDFLAGS -lI2util" - AC_SUBST(I2UTILLDFLAGS) AC_SUBST(I2UTILLIBS) AC_SUBST(I2UTILLIBDEPS) From c02b948c5a9b77f2ef78b29feba7dd4e759f0598 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 22 Mar 2017 14:28:13 -0400 Subject: [PATCH 08/10] remove web100 dependent test from TESTS --- src/Makefile.am | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index ec8f960..43b5093 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -28,7 +28,7 @@ endif bin_PROGRAMS = sbin_PROGRAMS = $(ADD_FAKEWWW) -noinst_PROGRAMS = +noinst_PROGRAMS = TESTS = if HAVE_WEB100 @@ -60,7 +60,6 @@ if HAVE_PCAP_H if HAVE_SSL if HAVE_JANSSON sbin_PROGRAMS += web100srv -TESTS += web100srv_unit_tests endif endif endif From 91ebeb63610818d91b3d0194cbfedd5556a8dab1 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 22 Mar 2017 14:29:34 -0400 Subject: [PATCH 09/10] remove unused tcp_stat_setbuf decl that breaks strict lint --- src/web100srv.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/web100srv.h b/src/web100srv.h index d324ab3..a6afb94 100644 --- a/src/web100srv.h +++ b/src/web100srv.h @@ -323,8 +323,6 @@ int tcp_stat_autotune(int sock, tcp_stat_agent* agent, tcp_stat_connection cn); int tcp_stat_init(char *VarFileName); void tcp_stat_middlebox(int sock, tcp_stat_agent* agent, tcp_stat_connection cn, char *results_keys, size_t results_keys_strlen, char *results_values, size_t results_strlen); -int tcp_stat_setbuff(int sock, tcp_stat_agent* agent, tcp_stat_connection cn, - int autotune);/* Not used so no web10g version */ void tcp_stat_get_data_recv(int sock, tcp_stat_agent* agent, tcp_stat_connection cn, int count_vars); From e3dceb1b235a3f1152253843ff2f8a143558249b Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Thu, 23 Mar 2017 20:21:57 -0400 Subject: [PATCH 10/10] Temp workaround for builder --- .travis.yml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5a875ad..53b6156 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,8 +8,13 @@ services: - docker script: -- docker pull gcr.io/mlab-pub/github-m-lab-builder:latest +# Temporarily use the git repo dev branch, instead of the google container registry. +# TODO - update this once the GCR builder is up to date. +- docker build -t build https://github.com/m-lab/builder.git#dev +- docker run build /root/ndt_build_and_test.sh dev + +#- docker pull gcr.io/mlab-pub/github-m-lab-builder:latest # Run basic unit tests that don't require web100 -- docker run gcr.io/mlab-pub/github-m-lab-builder /root/ndt_build_and_test.sh +#- docker run gcr.io/mlab-pub/github-m-lab-builder /root/ndt_build_and_test.sh -# TODO - collect coverage stats and export to coveralls. \ No newline at end of file +# TODO - collect coverage stats and export to coveralls.