[icinga-checkins] icinga.org: icinga-core/master: base/utils.c: Refactor my_fcopy() ( Andreas Ericsson)

git at icinga.org git at icinga.org
Wed May 19 13:41:53 CEST 2010


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

Author: Michael Friedrich <michael.friedrich at univie.ac.at>
Date:   Wed May 12 23:27:54 2010 +0200

base/utils.c: Refactor my_fcopy() (Andreas Ericsson)

[--snip--]
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.
[--snip--]

Wonderful patch! Perfect for Icinga too :)

fixes #427

---

 Changelog    |    1 +
 base/utils.c |  107 ++++++++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/Changelog b/Changelog
index 9610149..75119c6 100644
--- a/Changelog
+++ b/Changelog
@@ -45,6 +45,7 @@ FIXES
 * core: base/nebmods.c: Replace local file-copy hack with my_fcopy() (Andreas Ericsson)
 * core: enable compiler flag -Wall by default (base/cgi/common/idoutils)
 * core: fix open() error checking in move_check_result_to_queue() (Andreas Ericsson)
+* core: base/utils.c: Refactor my_fcopy() (Andreas Ericsson)
 
 * idoutils: Host DB inserts use string 'NULL\n' instead of NULL (William Preston)
 * idoutils: ndo2db_get_object_id fails to return existing IDs (William Preston)
diff --git a/base/utils.c b/base/utils.c
index ec1d929..6f30a00 100644
--- a/base/utils.c
+++ b/base/utils.c
@@ -3002,11 +3002,12 @@ int my_rename(char *source, char *dest){
 
 /* 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 rd_result = 0, wr_result = 0;
+        off_t tot_written = 0, tot_read = 0, buf_size = 0;
+        char *buf;
+        struct stat st;
 
 	/* make sure we have something */
 	if(source==NULL || dest==NULL)
@@ -3015,37 +3016,91 @@ int my_fcopy(char *source, char *dest){
 	/* unlink destination file first (not doing so can cause problems on network file systems like CIFS) */
 	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){
+                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 '%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;
+        }
 
-		/* 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);
-			}
+	/* open destination file for writing */
+        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));
+                close(source_fd);
+                free(buf);
+                return ERROR;
+        }
 
-		/* 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;
-			}
-		}
+        /* 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;
+        }
 
-	/* error opening destination file */
-	else{
-		close(dest_fd);
-		logit(NSLOG_RUNTIME_ERROR,TRUE,"Error: Unable to open file '%s' for writing: %s\n",dest,strerror(errno));
+
+        /* clean up irregardless of how things went */
+        close(source_fd);
+        close(dest_fd);
+        free(buf);
+ 
+        if (rd_result < 0 || wr_result < 0) {
+                /* don't leave half-written files around */
+                unlink(dest);
 		return ERROR;
-		}
+	}
 
 	return OK;
-        }
+}
 
 
 /******************************************************************/





More information about the icinga-checkins mailing list