From 417ee1393b425d5c7c998562e7fb492e28928643 Mon Sep 17 00:00:00 2001 From: Quincey Koziol Date: Tue, 26 Apr 2022 11:07:15 -0500 Subject: Correct concurrency bugs when running tests, along with a bugfix & small warning cleanup (#1683) * Correct concurrency bugs when running tests, along with a bugfix & small warning cleanup. * Committing clang-format changes Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- test/mirror_vfd.c | 185 +++++++++++++++++++++++++++++----- test/test_mirror.sh.in | 10 +- utils/mirror_vfd/mirror_server.c | 28 +++-- utils/mirror_vfd/mirror_server_stop.c | 13 ++- 4 files changed, 202 insertions(+), 34 deletions(-) diff --git a/test/mirror_vfd.c b/test/mirror_vfd.c index f66e6fc..c89a368 100644 --- a/test/mirror_vfd.c +++ b/test/mirror_vfd.c @@ -75,6 +75,25 @@ static unsigned int g_verbosity = DEFAULT_VERBOSITY; #define MIRR_MESG_SIZE 128 static char mesg[MIRR_MESG_SIZE + 1]; +/* ---------------------------------------------------------------------------- + * Structure: struct mt_opts + * + * Purpose: Convenience structure to hold options as parsed from the + * command line. + * + * `portno` (int) + * Port number, as received from arguments. + * + * `ip` (char *) + * IP address string as received from arguments. + * + * ---------------------------------------------------------------------------- + */ +struct mt_opts { + int portno; + char ip[H5FD_MIRROR_MAX_IP_LEN + 1]; +}; + /* Convenience structure for passing file names via helper functions. */ struct mirrortest_filenames { @@ -98,7 +117,8 @@ static herr_t _close_chunking_ids(unsigned min_dset, unsigned max_dset, hid_t *d static herr_t _populate_filepath(const char *dirname, const char *_basename, hid_t fapl_id, char *path_out, hbool_t h5suffix); -static hid_t create_mirroring_split_fapl(const char *_basename, struct mirrortest_filenames *names); +static hid_t create_mirroring_split_fapl(const char *_basename, struct mirrortest_filenames *names, + const struct mt_opts *opts); static void mybzero(void *dest, size_t size); @@ -1246,7 +1266,8 @@ error: * --------------------------------------------------------------------------- */ static hid_t -create_mirroring_split_fapl(const char *_basename, struct mirrortest_filenames *names) +create_mirroring_split_fapl(const char *_basename, struct mirrortest_filenames *names, + const struct mt_opts *opts) { H5FD_splitter_vfd_config_t splitter_config; H5FD_mirror_fapl_t mirror_conf; @@ -1274,8 +1295,8 @@ create_mirroring_split_fapl(const char *_basename, struct mirrortest_filenames * */ mirror_conf.magic = H5FD_MIRROR_FAPL_MAGIC; mirror_conf.version = H5FD_MIRROR_CURR_FAPL_T_VERSION; - mirror_conf.handshake_port = SERVER_HANDSHAKE_PORT; - if (HDstrncpy(mirror_conf.remote_ip, SERVER_IP_ADDRESS, H5FD_MIRROR_MAX_IP_LEN) == NULL) { + mirror_conf.handshake_port = opts->portno; + if (HDstrncpy(mirror_conf.remote_ip, opts->ip, H5FD_MIRROR_MAX_IP_LEN) == NULL) { TEST_ERROR; } splitter_config.wo_fapl_id = H5Pcreate(H5P_FILE_ACCESS); @@ -1354,7 +1375,7 @@ error: * --------------------------------------------------------------------------- */ static int -test_create_and_close(void) +test_create_and_close(const struct mt_opts *opts) { struct mirrortest_filenames names; hid_t file_id = H5I_INVALID_HID; @@ -1364,7 +1385,7 @@ test_create_and_close(void) /* Create FAPL for Splitter[sec2|mirror] */ - fapl_id = create_mirroring_split_fapl("basic_create", &names); + fapl_id = create_mirroring_split_fapl("basic_create", &names, opts); if (H5I_INVALID_HID == fapl_id) { TEST_ERROR; } @@ -1889,7 +1910,7 @@ error: * --------------------------------------------------------------------------- */ static int -test_basic_dataset_write(void) +test_basic_dataset_write(const struct mt_opts *opts) { struct mirrortest_filenames names; hid_t file_id = H5I_INVALID_HID; @@ -1906,7 +1927,7 @@ test_basic_dataset_write(void) /* Create FAPL for Splitter[sec2|mirror] */ - fapl_id = create_mirroring_split_fapl("basic_write", &names); + fapl_id = create_mirroring_split_fapl("basic_write", &names, opts); if (H5I_INVALID_HID == fapl_id) { TEST_ERROR; } @@ -2020,7 +2041,7 @@ error: * --------------------------------------------------------------------------- */ static int -test_chunked_dataset_write(void) +test_chunked_dataset_write(const struct mt_opts *opts) { struct mirrortest_filenames names; hid_t file_id = H5I_INVALID_HID; @@ -2030,7 +2051,7 @@ test_chunked_dataset_write(void) /* Create FAPL for Splitter[sec2|mirror] */ - fapl_id = create_mirroring_split_fapl("chunked_write", &names); + fapl_id = create_mirroring_split_fapl("chunked_write", &names, opts); if (H5I_INVALID_HID == fapl_id) { TEST_ERROR; } @@ -2134,7 +2155,7 @@ error: * --------------------------------------------------------------------------- */ static int -test_on_disk_zoo(void) +test_on_disk_zoo(const struct mt_opts *opts) { const char grp_name[] = "/only"; struct mirrortest_filenames names; @@ -2146,7 +2167,7 @@ test_on_disk_zoo(void) /* Create FAPL for Splitter[sec2|mirror] */ - fapl_id = create_mirroring_split_fapl("zoo", &names); + fapl_id = create_mirroring_split_fapl("zoo", &names, opts); if (H5I_INVALID_HID == fapl_id) { TEST_ERROR; } @@ -2248,7 +2269,7 @@ error: * --------------------------------------------------------------------------- */ static int -test_vanishing_datasets(void) +test_vanishing_datasets(const struct mt_opts *opts) { struct mirrortest_filenames names; hid_t file_id = H5I_INVALID_HID; @@ -2269,7 +2290,7 @@ test_vanishing_datasets(void) /* Create FAPL for Splitter[sec2|mirror] */ - fapl_id = create_mirroring_split_fapl("vanishing", &names); + fapl_id = create_mirroring_split_fapl("vanishing", &names, opts); if (H5I_INVALID_HID == fapl_id) { TEST_ERROR; } @@ -2416,7 +2437,7 @@ error: * --------------------------------------------------------------------------- */ static int -test_concurrent_access(void) +test_concurrent_access(const struct mt_opts *opts) { struct file_bundle { struct mirrortest_filenames names; @@ -2449,7 +2470,7 @@ test_concurrent_access(void) char _name[16] = ""; hid_t _fapl_id = H5I_INVALID_HID; HDsnprintf(_name, 15, "concurrent%d", i); - _fapl_id = create_mirroring_split_fapl(_name, &bundle[i].names); + _fapl_id = create_mirroring_split_fapl(_name, &bundle[i].names, opts); if (H5I_INVALID_HID == _fapl_id) { TEST_ERROR; } @@ -2560,6 +2581,111 @@ error: return -1; } /* end test_concurrent_access() */ +/* ---------------------------------------------------------------------------- + * Function: parse_args + * + * Purpose: Parse command-line arguments, populating the options struct + * pointer as appropriate. + * Default values will be set for unspecified options. + * + * Return: 0 on success, negative (-1) if error. + * ---------------------------------------------------------------------------- + */ +static int +parse_args(int argc, char **argv, struct mt_opts *opts) +{ + int i = 0; + + opts->portno = SERVER_HANDSHAKE_PORT; + HDstrncpy(opts->ip, SERVER_IP_ADDRESS, H5FD_MIRROR_MAX_IP_LEN); + + for (i = 1; i < argc; i++) { /* start with first possible option argument */ + if (!HDstrncmp(argv[i], "--ip=", 5)) { + HDstrncpy(opts->ip, argv[i] + 5, H5FD_MIRROR_MAX_IP_LEN); + } + else if (!HDstrncmp(argv[i], "--port=", 7)) { + opts->portno = HDatoi(argv[i] + 7); + } + else { + HDprintf("Unrecognized option: '%s'\n", argv[i]); + return -1; + } + } /* end for each argument from command line */ + + /* auto-replace 'localhost' with numeric IP */ + if (!HDstrncmp(opts->ip, "localhost", 10)) { /* include null terminator */ + HDstrncpy(opts->ip, "127.0.0.1", H5FD_MIRROR_MAX_IP_LEN); + } + + return 0; +} /* end parse_args() */ + +/* ---------------------------------------------------------------------------- + * Function: confirm_server + * + * Purpose: Create socket and confirm remote server is available. + * + * Return: 0 on success, negative (-1) if error. + * ---------------------------------------------------------------------------- + */ +static int +confirm_server(struct mt_opts *opts) +{ + char mybuf[16]; + int live_socket; + struct sockaddr_in target_addr; + unsigned attempt = 0; + + live_socket = HDsocket(AF_INET, SOCK_STREAM, 0); + if (live_socket < 0) { + HDprintf("ERROR socket()\n"); + return -1; + } + + target_addr.sin_family = AF_INET; + target_addr.sin_port = HDhtons((uint16_t)opts->portno); + target_addr.sin_addr.s_addr = HDinet_addr(opts->ip); + HDmemset(target_addr.sin_zero, '\0', sizeof(target_addr.sin_zero)); + + while (1) { + if (HDconnect(live_socket, (struct sockaddr *)&target_addr, (socklen_t)sizeof(target_addr)) < 0) { + if (attempt > 10) { + HDprintf("ERROR connect() (%d)\n%s\n", errno, HDstrerror(errno)); + return -1; + } + attempt++; + HDsleep(1); + HDprintf("attempt #%u: ERROR connect() (%d)\n%s\n", attempt, errno, HDstrerror(errno)); + } + else { + break; + } + } + + /* Request confirmation from the server */ + if (HDwrite(live_socket, "CONFIRM", 8) == -1) { + HDprintf("ERROR write() (%d)\n%s\n", errno, HDstrerror(errno)); + return -1; + } + + /* Read & verify response from port connection. */ + if (HDread(live_socket, &mybuf, sizeof(mybuf)) == -1) { + HDprintf("ERROR read() can't receive data\n"); + return -1; + } + if (HDstrncmp("ALIVE", mybuf, 6)) { + HDprintf("ERROR read() didn't receive data from server\n"); + return -1; + } + + if (HDclose(live_socket) < 0) { + HDprintf("ERROR close() can't close socket\n"); + return -1; + } + + return 0; +} /* end confirm_server() */ + /* --------------------------------------------------------------------------- * Function: main * @@ -2573,9 +2699,10 @@ error: * --------------------------------------------------------------------------- */ int -main(void) +main(int argc, char **argv) { - int nerrors = 0; + struct mt_opts opts; + int nerrors = 0; h5_reset(); @@ -2599,6 +2726,16 @@ main(void) } } + if (parse_args(argc, argv, &opts) < 0) { + HDprintf("Unable to parse arguments\n"); + HDexit(EXIT_FAILURE); + } + + if (confirm_server(&opts) < 0) { + HDprintf("Unable to confirm server is running\n"); + HDexit(EXIT_FAILURE); + } + /* -------------------- */ /* TESTS */ /* Tests return negative values; `-=' increments nerrors count */ @@ -2606,12 +2743,12 @@ main(void) if (nerrors == 0) { nerrors -= test_fapl_configuration(); nerrors -= test_xmit_encode_decode(); - nerrors -= test_create_and_close(); - nerrors -= test_basic_dataset_write(); - nerrors -= test_chunked_dataset_write(); - nerrors -= test_on_disk_zoo(); - nerrors -= test_vanishing_datasets(); - nerrors -= test_concurrent_access(); + nerrors -= test_create_and_close(&opts); + nerrors -= test_basic_dataset_write(&opts); + nerrors -= test_chunked_dataset_write(&opts); + nerrors -= test_on_disk_zoo(&opts); + nerrors -= test_vanishing_datasets(&opts); + nerrors -= test_concurrent_access(&opts); } if (nerrors) { diff --git a/test/test_mirror.sh.in b/test/test_mirror.sh.in index 9713f89..fbc7ede 100644 --- a/test/test_mirror.sh.in +++ b/test/test_mirror.sh.in @@ -22,7 +22,10 @@ nerrors=0 SERVER_VERBOSITY="--verbosity=1" -SERVER_PORT="--port=3000" +# Choose random ephemeral port number +RANDOM_PORT=$[ $RANDOM % 16384 + 49152 ] +echo "Using port: $RANDOM_PORT" +SERVER_PORT="--port=$RANDOM_PORT" ############################################################################### @@ -83,12 +86,15 @@ echo "Launching Mirror Server" SERVER_ARGS="$SERVER_PORT $SERVER_VERBOSITY" ./mirror_server $SERVER_ARGS & -./mirror_vfd +./mirror_vfd $SERVER_PORT nerrors=$? echo "Stopping Mirror Server" ./mirror_server_stop $SERVER_PORT +# Wait for background server process to exit +wait + ############################################################################### ## Report and exit ############################################################################### diff --git a/utils/mirror_vfd/mirror_server.c b/utils/mirror_vfd/mirror_server.c index 5381d95..d6b5254 100644 --- a/utils/mirror_vfd/mirror_server.c +++ b/utils/mirror_vfd/mirror_server.c @@ -46,11 +46,8 @@ #ifdef H5_HAVE_MIRROR_VFD -#define MAXBUF 2048 /* max buffer length. */ -#define LISTENQ 80 /* max pending mirrorS requests */ -#define DEFAULT_PORT 3000 /* default listening port */ -#define MAX_PORT_LOOPS 20 /* max iteratations through port range */ -#define PORT_LOOP_RETRY_DELAY 1 /* seconds to wait between port scans */ +#define LISTENQ 80 /* max pending mirrorS requests */ +#define DEFAULT_PORT 3000 /* default listening port */ /* semi-unique "magic" numbers to sanity-check structure pointers */ #define OP_ARGS_MAGIC 0xCF074379u @@ -211,8 +208,8 @@ parse_args(int argc, char **argv, struct op_args *args_out) return -1; } - /* Loop over arguments after program name and writer_path */ - for (i = 2; i < argc; i++) { + /* Loop over arguments after program name */ + for (i = 1; i < argc; i++) { if (!HDstrncmp(argv[i], "-h", 3) || !HDstrncmp(argv[i], "--help", 7)) { mirror_log(NULL, V_INFO, "found help argument"); args_out->help = 1; @@ -536,10 +533,27 @@ handle_requests(struct server_run *run) if (!HDstrncmp("SHUTDOWN", mybuf, 8)) { /* Stop operation if told to stop */ mirror_log(run->loginfo, V_INFO, "received SHUTDOWN!", ret); + + /* Confirm operation */ + if ((ret = HDwrite(connfd, "CLOSING", 8)) < 0) { + mirror_log(run->loginfo, V_ERR, "write:%d", ret); + HDclose(connfd); + connfd = -1; + goto error; + } + HDclose(connfd); connfd = -1; goto done; } /* end if explicit "SHUTDOWN" directive */ + if (!HDstrncmp("CONFIRM", mybuf, 7)) { + /* Confirm operation */ + if ((ret = HDwrite(connfd, "ALIVE", 6)) < 0) { + mirror_log(run->loginfo, V_ERR, "write:%d", ret); + goto error; + } + HDclose(connfd); + } /* end if "CONFIRM" directive */ else if (H5FD_MIRROR_XMIT_OPEN_SIZE == ret) { H5FD_mirror_xmit_open_t xopen; diff --git a/utils/mirror_vfd/mirror_server_stop.c b/utils/mirror_vfd/mirror_server_stop.c index 024b33a..44386bf 100644 --- a/utils/mirror_vfd/mirror_server_stop.c +++ b/utils/mirror_vfd/mirror_server_stop.c @@ -60,7 +60,7 @@ struct mshs_opts { static void usage(void) { - HDprintf("mirror_server_halten_sie [options]\n" + HDprintf("mirror_server_stop [options]\n" "System-independent Mirror Server shutdown program.\n" "Sends shutdown message to Mirror Server at given IP:port\n" "\n" @@ -128,6 +128,7 @@ parse_args(int argc, char **argv, struct mshs_opts *opts) static int send_shutdown(struct mshs_opts *opts) { + char mybuf[16]; int live_socket; struct sockaddr_in target_addr; @@ -157,6 +158,16 @@ send_shutdown(struct mshs_opts *opts) return -1; } + /* Read & verify response from port connection. */ + if (HDread(live_socket, &mybuf, sizeof(mybuf)) == -1) { + HDprintf("ERROR read() can't receive data\n"); + return -1; + } + if (HDstrncmp("CLOSING", mybuf, 8)) { + HDprintf("ERROR read() didn't receive data from server\n"); + return -1; + } + if (HDclose(live_socket) < 0) { HDprintf("ERROR close() can't close socket\n"); return -1; -- cgit v0.12