Updating how we file datapatterns work

src/base/target_cleanup.c

@@ -69,7 +69,6 @@ xdd_target_thread_cleanup(target_data_t *tdp) {
             /* If the file target opend a file for the data pattern we must close
             * the files and clean up the data_pattern buffer
             */
-            close(tdp->dpp_fd);
             free(tdp->td_dpp->data_pattern);
         }
         free(tdp->td_dpp);

src/client/parse_func.c

@@ -555,6 +555,8 @@ 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 = -1;
+
 		if (argc <= args+2) {
 			fprintf(xgp->errout, "%s: not enough arguments specified for the option '-datapattern'\n", xgp->progname);
 			return(0);
@@ -566,61 +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);
-			}
-			tdp->dpp_fd = open(tdp->td_dpp->data_pattern_filename, O_RDONLY);
-			if (tdp->dpp_fd < 0) {
-				fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, tdp->td_dpp->data_pattern_filename);
+			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(tdp->dpp_fd, tdp->td_dpp->data_pattern, tdp->td_dpp->data_pattern_length, 0);
 		} 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);
 					}
-					tdp->dpp_fd = open(tdp->td_dpp->data_pattern_filename, O_RDONLY);
-					if (tdp->dpp_fd < 0) {
-						fprintf(xgp->errout, "%s: could not open %s\n", xgp->progname, tdp->td_dpp->data_pattern_filename);
+				}
+				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(tdp->dpp_fd, tdp->td_dpp->data_pattern, tdp->td_dpp->data_pattern_length, 0);
 					i++;
 					tdp = planp->target_datap[i];
 				}
+				close(dp_fd);
 			}
 		}
 	} else if (strcmp(pattern_type, "sequenced") == 0) {

src/common/datapatterns.c

@@ -257,22 +257,92 @@ 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;
+	}
+
+	size_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 %zu bytes but got %zu",
+			xgp->progname, filename, dp->data_pattern_length, bytes);
+			goto error;
+	}
+
+	return 0;
+
+error:
+	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

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

src/common/xint_td.h

@@ -156,7 +156,6 @@ struct xint_target_data {
 	struct xint_e2e				*td_e2ep;			// Pointer to the e2e struct when needed
 	struct xint_extended_stats	*td_esp;			// Extended Stats Structure Pointer
 	struct xint_triggers		*td_trigp;			// Triggers Structure Pointer
-	int                         dpp_fd;             // File descriptor for file data pattern
 	struct xint_data_pattern	*td_dpp;			// Data Pattern Structure Pointer
 	struct xint_raw				*td_rawp;          	// RAW Data Structure Pointer
 	struct lockstep				*td_lsp;			// Pointer to the lockstep structure used by the lockstep option

tests/functional/CMakeLists.txt

@@ -2,6 +2,7 @@ set(FUNCTIONAL
     test_xdd_createnewfiles.sh
     test_xdd_createnewfiles2.sh
     test_xdd_dryrun.sh
+    test_xdd_file_datapattern.sh
     test_xdd_heartbeat_byte.sh
     test_xdd_heartbeat_elapsed.sh
     test_xdd_heartbeat_lf.sh

tests/functional/test_xdd_file_datapattern.sh

@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+#
+# Acceptance test for XDD.
+#
+# Validate the funtionality of -datapattern file/wholefile option by creating a file, setting the datapattern
+# from that file and then verifying the contents match
+#
+# Description - terminates XDD after a given amount of seconds have passed
+#
+# Get absolute path to script
+SCRIPT=${BASH_SOURCE[0]}
+SCRIPTPATH=$(dirname "${SCRIPT}")
+
+# Source the test configuration environment
+source "${SCRIPTPATH}"/../test_config
+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"
+
+# Create datapattern input file using dd
+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 ! 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
+# 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 ! 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
+rm "${input_file}" "${output_file}"
+
+# Test passed
+finalize_test 0