[icinga-checkins] icinga.org: icinga-core/rbartels/1.3: base/utils: Refactor my_fcopy() ( Andreas Ericsson) #427

git at icinga.org git at icinga.org
Thu Nov 11 19:54:25 CET 2010


Module: icinga-core
Branch: rbartels/1.3
Commit: e26f31b57c36fea662690979eba208ce2be76a84
URL:    https://git.icinga.org/?p=icinga-core.git;a=commit;h=e26f31b57c36fea662690979eba208ce2be76a84

Author: Michael Friedrich <michael.friedrich at univie.ac.at>
Date:   Mon Oct 25 12:54:43 2010 +0200

base/utils: Refactor my_fcopy() (Andreas Ericsson) #427

This patch provides some much-needed fixing to my_fcopy() by a
more or less complete rewrite of the function.

Previously it didn't use to check that it had, indeed, written
all data to the destination file. This is now done.

It also used a ridiculously small buffer of 1K, which is clearly
suboptimal for the task. A value of 128K or filesize(source),
whichever is smallest, should allow us to utilize the disk's
cache better without being so large that it drops out of the L2
cache.

It would also ignore the error conditions of "EINTR" and "EAGAIN",
which can happen if we receive a signal in the middle of reading
or writing, and, on failure after having opened the destination
file, it would leave the half-written (or empty) file around,
which could cause a major build-up of checkresult files in the
spool directory after some time.

It was triggered by the fact that my_fcopy() used to check the
return code of open() wrong, and spiralled slightly out of
control from there.

All these issues are addressed with the new code, which also
uses the "check for error and return if so" style of coding to
prevent indentation going to 5 levels deep or more.

Breaking it into two separate functions also makes it usable
when race-lessly creating temporary files using functions
like mkstemp().

Signed-off-by: Andreas Ericsson <ae at op5.se>

Author: Andreas Ericsson <ae at op5.se>

refs #427

---

 Changelog        |    6 +++
 base/utils.c     |  120 ++++++++++++++++++++++++++++++++++++++++++------------
 include/icinga.h |    1 +
 3 files changed, 101 insertions(+), 26 deletions(-)

diff --git a/Changelog b/Changelog
index e897ae5..f220157 100644
--- a/Changelog
+++ b/Changelog
@@ -4,6 +4,12 @@ Icinga 1.3.x Change Log
 
 1.3.0 - 16/02/2011
 
+ENHANCEMENTS
+
+
+FIXES
+* core: base/utils: Refactor my_fcopy() (Andreas Ericsson) #427
+
 
 1.2.1 - 25/10/2010
 
diff --git a/base/utils.c b/base/utils.c
index e38b2c2..c952724 100644
--- a/base/utils.c
+++ b/base/utils.c
@@ -3381,14 +3381,98 @@ int my_rename(char *source, char *dest){
 	return rename_result;
         }
 
+/*
+ * copy a file from the path at source to the already opened
+ * destination file dest.
+ * This is handy when creating tempfiles with mkstemp()
+ */
+int my_fdcopy(char *source, char *dest, int dest_fd){
+	int source_fd, rd_result = 0, wr_result = 0;
+	off_t tot_written = 0, tot_read = 0, buf_size = 0;
+	struct stat st;
+	char *buf;
+
+	/* open source file for reading */
+	if((source_fd=open(source,O_RDONLY,0644)) < 0){
+		logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: Unable to open file '%s' for reading: %s\n",source,strerror(errno));
+		return ERROR;
+	}
+
+	/*
+	 * find out how large the source-file is so we can be sure
+	 * we've written all of it
+	 */
+	if (fstat(source_fd, &st) < 0) {
+		logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: Unable to stat source file '%s' for my_fcopy(): %s\n", source, strerror(errno));
+		close(source_fd);
+		return ERROR;
+	}
+
+	/*
+	 * If the file is huge, read it and write it in chunks.
+	 * This value (128K) is the result of "pick-one-at-random"
+	 * with some minimal testing and may not be optimal for all
+	 * hardware setups, but it should work ok for most. It's
+	 * faster than 1K buffers and 1M buffers, so change at your
+	 * own peril. Note that it's useful to make it fit in the L2
+	 * cache, so larger isn't necessarily better.
+	 */
+	buf_size = st.st_size > 128 << 10 ? 128 << 10 : st.st_size;
+	buf = malloc(buf_size);
+	if (!buf) {
+		logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: Unable to malloc(%ld) bytes: %s\n", buf_size, strerror(errno));
+		close(source_fd);
+		return ERROR;
+	}
+	/* most of the times, this loop will be gone through once */
+	while (tot_written < st.st_size) {
+		int loop_wr = 0;
+
+		rd_result = read(source_fd, buf, buf_size);
+		if (rd_result < 0) {
+			if (errno == EAGAIN || errno == EINTR)
+				continue;
+			logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: my_fcopy() failed to read from '%s': %s\n", source, strerror(errno));
+			break;
+		}
+		tot_read += rd_result;
+
+		while (loop_wr < rd_result) {
+			wr_result = write(dest_fd, buf + loop_wr, rd_result - loop_wr);
+
+			if (wr_result < 0) {
+				if (errno == EAGAIN || errno == EINTR)
+					continue;
+				logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: my_fcopy() failed to write to '%s': %s\n", dest, strerror(errno));
+				break;
+			}
+			loop_wr += wr_result;
+		}
+		if (wr_result < 0)
+			break;
+		tot_written += loop_wr;
+	}
+
+	/*
+	 * clean up irregardless of how things went. dest_fd comes from
+	 * our caller, so we mustn't close it.
+	 */
+	close(source_fd);
+	free(buf);
+
+	if (rd_result < 0 || wr_result < 0) {
+		/* don't leave half-written files around */
+		unlink(dest);
+		return ERROR;
+	}
+
+	return OK;
+}
 
 
 /* copies a file */
 int my_fcopy(char *source, char *dest){
-	char buffer[MAX_INPUT_BUFFER]={0};
-	int source_fd=-1;
-	int dest_fd=-1;
-	int bytes_read=0;
+	int dest_fd, result;
 
 	/* make sure we have something */
 	if(source==NULL || dest==NULL)
@@ -3398,32 +3482,16 @@ int my_fcopy(char *source, char *dest){
 	unlink(dest);
 
         /* open destination file for writing */
-        if((dest_fd=open(dest,O_WRONLY|O_TRUNC|O_CREAT|O_APPEND,0644))>0){
-                /* open source file for reading */
-                if((source_fd=open(source,O_RDONLY,0644))>0){
-                        /* copy file contents */
-                        while((bytes_read=read(source_fd,buffer,sizeof(buffer)))>0)
-                                write(dest_fd,buffer,bytes_read);
-                        close(source_fd);
-                        close(dest_fd);
-                        }
-                /* error opening the source file */
-                else{
-                        close(dest_fd);
-                        logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: Unable to open file '%s' for reading: %s\n",source,strerror(errno));
-                        return ERROR;
-                        }
-                }
-        /* error opening destination file */
-        else{
-                close(dest_fd);
+	if((dest_fd=open(dest,O_WRONLY|O_TRUNC|O_CREAT|O_APPEND,0644)) < 0){
                 logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: Unable to open file '%s' for writing: %s\n",dest,strerror(errno));
 		return ERROR;
-                }
-
-	return OK;
         }
 
+	result = my_fdcopy(source, dest, dest_fd);
+	close(dest_fd);
+	return result;
+}
+
 
 /******************************************************************/
 /******************** DYNAMIC BUFFER FUNCTIONS ********************/
diff --git a/include/icinga.h b/include/icinga.h
index 70ef687..8f90ef3 100644
--- a/include/icinga.h
+++ b/include/icinga.h
@@ -572,6 +572,7 @@ char *escape_newlines(char *);
 int contains_illegal_object_chars(char *);		/* tests whether or not an object name (host, service, etc.) contains illegal characters */
 int my_rename(char *,char *);                           /* renames a file - works across filesystems */
 int my_fcopy(char *,char *);                            /* copies a file - works across filesystems */
+int my_fdcopy(char *, char *, int);                     /* copies a named source to an already opened destination file */
 int get_raw_command_line(command *,char *,char **,int);    	/* given a raw command line, determine the actual command to run */
 int check_time_against_period(time_t,timeperiod *);	/* check to see if a specific time is covered by a time period */
 int is_daterange_single_day(daterange *);





More information about the icinga-checkins mailing list