From aee48376e27193632e7212a7fe4a13fe18e12434 Mon Sep 17 00:00:00 2001 From: Brian Atkinson Date: Tue, 19 Mar 2024 18:56:44 -0600 Subject: [PATCH] Addressing feedback on PR Addressing issues I had in my initial PR based on review feedback. I also did some general code clean up for parse_func.c. This commit needs to be squashed into the original commit if this PR is accepted. I will take care of that before the merge. Signed-off-by: Brian Atkinson --- src/client/parse_func.c | 67 ++++++--------- src/common/datapatterns.c | 82 +++++++++++++++++-- src/common/xint_prototypes.h | 4 +- tests/functional/test_xdd_file_datapattern.sh | 17 ++-- 4 files changed, 118 insertions(+), 52 deletions(-) diff --git a/src/client/parse_func.c b/src/client/parse_func.c index afb989b6..00958b1e 100644 --- a/src/client/parse_func.c +++ b/src/client/parse_func.c @@ -555,7 +555,7 @@ xddfunc_datapattern(xdd_plan_t *planp, int32_t argc, char *argv[], uint32_t flag } } else if (strcmp(pattern_type, "file") == 0 || strcmp(pattern_type, "wholefile") == 0) { retval++; - int dp_fd; + int dp_fd = -1; if (argc <= args+2) { fprintf(xgp->errout, "%s: not enough arguments specified for the option '-datapattern'\n", xgp->progname); @@ -568,63 +568,48 @@ xddfunc_datapattern(xdd_plan_t *planp, int32_t argc, char *argv[], uint32_t flag (char *)argv[args+2]); return(0); } - pattern_length = stat_buf.st_size; + } if (tdp) { /* set option for specific target */ if (strcmp(pattern_type, "file") == 0) { tdp->td_dpp->data_pattern_options |= DP_FILE_PATTERN; } else {// wholefile tdp->td_dpp->data_pattern_options |= DP_WHOLEFILE_PATTERN; + if (!xdd_datapattern_wholefile_enough_ram(tdp, argv[args+2])) { + return(0); + } } - tdp->td_dpp->data_pattern_filename = (char *)argv[args+2]; - tdp->td_dpp->data_pattern_length = pattern_length; - if (!xdd_datapattern_wholefile_enough_ram(tdp)) { - fprintf(xgp->errout, "%s: file %s can not be loaded into memory because 50%% of RAM is " - "currently unavailable\n", - xgp->progname, - (char *)argv[args+2]); - return(0); - } - dp_fd = open(tdp->td_dpp->data_pattern_filename, O_RDONLY); - if (dp_fd < 0) { - fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, tdp->td_dpp->data_pattern_filename); - return(0); + tdp->td_dpp->data_pattern_length = stat_buf.st_size; + if(xdd_set_datapattern_from_filename(tdp, argv[args+2])) { + return (0); } - tdp->td_dpp->data_pattern = (unsigned char *)malloc(sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1)); - memset(tdp->td_dpp->data_pattern, '\0', sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1)); - pread(dp_fd, tdp->td_dpp->data_pattern, tdp->td_dpp->data_pattern_length, 0); - close(dp_fd); - } else {// Put this option into all Targets + } else {// Put this option into all Targets if (flags & XDD_PARSE_PHASE2) { tdp = planp->target_datap[0]; + tdp->td_dpp->data_pattern_length = stat_buf.st_size; i = 0; - while (tdp) { - if (strcmp(pattern_type, "file") == 0) { - tdp->td_dpp->data_pattern_options |= DP_FILE_PATTERN; - } else {//wholefile - tdp->td_dpp->data_pattern_options |= DP_WHOLEFILE_PATTERN; - } - tdp->td_dpp->data_pattern_filename = (char *)argv[args+2]; - tdp->td_dpp->data_pattern_length = pattern_length; - if (!xdd_datapattern_wholefile_enough_ram(tdp)) { - fprintf(xgp->errout, "%s: the file %s is larger than 50%% of total RAM\n", - xgp->progname, - (char *)argv[args+2]); + dp_fd = open(argv[args+2], O_RDONLY); + if (dp_fd < 0) { + fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, argv[args+2]); + return (0); + } + if (strcmp(pattern_type, "file") == 0) { + tdp->td_dpp->data_pattern_options |= DP_FILE_PATTERN; + } else {//wholefile + tdp->td_dpp->data_pattern_options |= DP_WHOLEFILE_PATTERN; + if (!xdd_datapattern_wholefile_enough_ram(tdp, argv[args+2])) { return(0); } - dp_fd = open(tdp->td_dpp->data_pattern_filename, O_RDONLY); - if (dp_fd < 0) { - fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, tdp->td_dpp->data_pattern_filename); - return(0); + } + while (tdp) { + if (xdd_set_datapattern_from_file_descriptor(tdp, dp_fd, argv[args+2])) { + close(dp_fd); + return (0); } - tdp->td_dpp->data_pattern_length = pattern_length; - tdp->td_dpp->data_pattern = (unsigned char *)malloc(sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1)); - memset(tdp->td_dpp->data_pattern, '\0', sizeof(unsigned char) * (tdp->td_dpp->data_pattern_length + 1)); - pread(dp_fd, tdp->td_dpp->data_pattern, tdp->td_dpp->data_pattern_length, 0); - close(dp_fd); i++; tdp = planp->target_datap[i]; } + close(dp_fd); } } } else if (strcmp(pattern_type, "sequenced") == 0) { diff --git a/src/common/datapatterns.c b/src/common/datapatterns.c index a2a66b88..dab1fecc 100644 --- a/src/common/datapatterns.c +++ b/src/common/datapatterns.c @@ -257,22 +257,94 @@ xdd_datapattern_fill(worker_data_t *wdp) { * WHOLEFILE_MAX_SIZE_RAM. */ int -xdd_datapattern_wholefile_enough_ram(target_data_t *tdp) { - int ret = TRUE; +xdd_datapattern_wholefile_enough_ram(target_data_t *tdp, const char *filename) { struct sysinfo info; size_t file_size = tdp->td_dpp->data_pattern_length; + assert(tdp->td_dpp->data_pattern_options & DP_WHOLEFILE_PATTERN); + // Short circuit empty files if (!file_size) - return FALSE; + goto error; sysinfo(&info); if (file_size > (info.freeram * WHOLEFILE_MAX_SIZE_RAM)) - ret = FALSE; + goto error;; - return ret; + return TRUE; + +error: + fprintf(xgp->errout, "%s: file %s can not be loaded into memory because 50%% of RAM is " + "currently unavailable\n", + xgp->progname, + filename); + + return FALSE; } // End of xdd_datapattern_wholefile_enough_ram() +/*----------------------------------------------------------------------------*/ +/* xdd_set_datapattern_from_filename() - This subroutine will set the targets + * data_pattern from a file. The targets td_dpp->data_pattern_length must be + * set before calling this. On error returns 1, and on success returns 0. + */ +int +xdd_set_datapattern_from_filename(target_data_t *tdp, char *filename) { + struct xint_data_pattern *dp = tdp->td_dpp; + int ret = 0; + + int fd = open(filename, O_RDONLY); + if (fd < 0 ) { + fprintf(xgp->errout, "%s, could not open %s\n", xgp->progname, filename); + return 1; + } + + dp->data_pattern_filename = filename; + ret = xdd_set_datapattern_from_file_descriptor(tdp, fd, filename); + close (fd); + + return ret; +} // End of xdd_set_datapattern_from_filename() + +/*----------------------------------------------------------------------------*/ +/* xdd_set_datapattern_from_file_descriptor() - This subroutine will set the + * targets data_pattern from an open file descriptor. The targets + * td_dpp->data_pattern_length must be set before calling this. On error returns 1, + * and on success returns 0. + */ +int +xdd_set_datapattern_from_file_descriptor(target_data_t *tdp, int fd, char *filename) { + struct xint_data_pattern *dp = tdp->td_dpp; + + // Short circuit if the file is not open + if (fd < 0) { + return 1; + } + + dp->data_pattern_filename = filename; + dp->data_pattern = (unsigned char *)malloc(sizeof(unsigned char) * dp->data_pattern_length + 1) ; + if (!dp->data_pattern) { + fprintf(xgp->errout, "%s: could not allocate datapattern buffer for target %d", + xgp->progname, tdp->td_target_number); + goto error; + } + + memset(dp->data_pattern, '\0', sizeof(unsigned char) * dp->data_pattern_length + 1); + ssize_t bytes = pread(fd, dp->data_pattern, dp->data_pattern_length, 0); + if (bytes != dp->data_pattern_length) { + fprintf(xgp->errout, "%s: short read from file %s when setting target datapattern. " + "Espected %d bytes but got %d", + xgp->progname, filename, dp->data_pattern_length, bytes); + goto error; + } + + return 0; +error: + if (dp->data_pattern) { + free(dp->data_pattern); + } + return 1; +} // End of xdd_set_datapattern_from_file_descriptor() + /*----------------------------------------------------------------------------*/ /* xdd_datapattern_get_datap_from_offset() - This subroutine will return the * beginning of a the buffer from the target's tp_dpp->data_pattern based on diff --git a/src/common/xint_prototypes.h b/src/common/xint_prototypes.h index 1186e91d..5a4a4545 100644 --- a/src/common/xint_prototypes.h +++ b/src/common/xint_prototypes.h @@ -35,7 +35,9 @@ int32_t xdd_barrier(struct xdd_barrier *bp, xdd_occupant_t *occupantp, char owne // datapatterns.c void xdd_datapattern_buffer_init(worker_data_t *wdp); void xdd_datapattern_fill(worker_data_t *wdp); -int xdd_datapattern_wholefile_enough_ram(target_data_t *tdp); +int xdd_datapattern_wholefile_enough_ram(target_data_t *tdp, const char *filename); +int xdd_set_datapattern_from_filename(target_data_t *tdp, char *filename); +int xdd_set_datapattern_from_file_descriptor(target_data_t *tdp, int fd, char *filename); unsigned char *xdd_datapattern_get_datap_from_offset(worker_data_t *wdp); // debug.c diff --git a/tests/functional/test_xdd_file_datapattern.sh b/tests/functional/test_xdd_file_datapattern.sh index f8af07dd..0ade7dc6 100755 --- a/tests/functional/test_xdd_file_datapattern.sh +++ b/tests/functional/test_xdd_file_datapattern.sh @@ -18,9 +18,10 @@ source "${SCRIPTPATH}"/../common.sh # Perform pre-test initialize_test test_dir="${XDDTEST_LOCAL_MOUNT}/${TESTNAME}" +log_file="$(get_log_file)" -input_file=$test_dir/data1.dat -output_file=$test_dir/data2.dat +input_file="${test_dir}/data1.dat" +output_file="${test_dir}/data2.dat" # Create datapattern input file using dd dd if=/dev/urandom of="${input_file}" bs=4k count=2 @@ -28,18 +29,24 @@ dd if=/dev/urandom of="${input_file}" bs=4k count=2 # Write out file using xdd with -datapattern file "${XDDTEST_XDD_EXE}" -op write -reqsize 8 -numreqs 1 -datapattern file "${input_file}" -targets 1 "${output_file}" -qd 1 -passes 1 # Verify the contents of the file match -if ! diff "${input_file}" "${output_file}" +if ! cmp "${input_file}" "${output_file}" then + echo "Error when comparing files ${input_file} and ${output_file} with -datapattern file" > "${log_file}" + cmp "${input_file}" "${output_file}" >> "${log_file}" rm "${input_file}" "${output_file}" finalize_test 1 "Issue with setting datapattern from file with -datapattern file. Contents don't match." fi rm "${output_file}" # Write out file using xdd with -datapattern wholefile -"${XDDTEST_XDD_EXE}" -op write -reqsize 8 -numreqs 1 -datapattern wholefile "${input_file}" -targets 1 "${output_file}" -qd 1 -passes 1 +# In this caes we will use two threads to write out the data in serial ordering as the +# contents of the file will be distributed evenly between both threads. +"${XDDTEST_XDD_EXE}" -op write -reqsize 4 -numreqs 2 -datapattern wholefile "${input_file}" -targets 1 "${output_file}" -serialordering -qd 2 -passes 1 # Verify the contents of the file match -if ! diff "${input_file}" "${output_file}" +if ! cmp "${input_file}" "${output_file}" then + echo "Error when comparing files ${input_file} and ${output_file} with -datapattern wholefile" > "${log_file}" + cmp "${input_file}" "${output_file}" >> "${log_file}" rm "${input_file}" "${output_file}" finalize_test 1 "Issue with setting datapattern from file with -datapattern file. Contents don't match." fi