Re: 1.1.7 rlm_detail locking is broken

3 Dec 2007

On 11/29/07 17:46, Brian De Wolf wrote:
...
Expect a patch, soon enough...
Here's the patch, as promised.  Now, instead of radrelay's previous behavior, 
radrelay will attempt to open and lock <detail>.work.  If it can, it will then 
read/transmit all of the entries and delete <detail>.work.  If it can't lock, it 
will simply try locking later.  If it doesn't exist, it will check if <detail> 
has entries and if so it will move <detail> to <detail>.work (and make a new 
<detail>).

It seems to move faster, but that's because I removed some of the sleeps to make 
sure writing had completed since that should be handled by the locks now. 
Luckily, my relay destinations aren't production yet, so I can keep testing with 
a decent load.

How does it look?

--- src/main/radrelay.c.orig	2007-03-16 06:22:03.000000000 -0700
+++ src/main/radrelay.c	2007-12-03 12:24:46.000000000 -0800
@@ -202,8 +202,6 @@
 
 /*
  *	Read one request from the detail file.
- *	Note that the file is locked during the read, and that
- *	we return *with the file locked* if we reach end-of-file.
  *
  *	STATE_EMPTY:	Slot is empty.
  *	STATE_BUSY1:	Looking for start of a detail record (timestamp)
@@ -219,8 +217,6 @@
 	char key[32], val[32];
 	int skip;
 	long fpos;
-	int x;
-	unsigned int i = 0;
 
 	/* Never happens */
 	if (r_req->state == STATE_FULL)
@@ -230,21 +226,7 @@
 		r_req->state = STATE_BUSY1;
 	}
 
-	/*
-	 * Try to lock the detail-file.
-	 * If lockf is used we want to lock the _whole_ file, hence the
-	 * fseek to the start of the file.
-	 */
 	fpos = ftell(fp);
-	fseek(fp, 0L, SEEK_SET);
-	do {
-		x = rad_lockfd_nonblock(fileno(fp), 0);
-		if (x == -1)
-			ms_sleep(100);
-	} while (x == -1 && i++ < 20);
-
-	if (x == -1)
-		return 0;
 
 redo:
 	s = NULL;
@@ -359,11 +341,6 @@
 			return EOF;
 	}
 
-	fpos = ftell(fp);
-	fseek(fp, 0L, SEEK_SET);
-	rad_unlockfd(fileno(fp), 0);
-	fseek(fp, fpos, SEEK_SET);
-
 	return 0;
 }
 
@@ -537,11 +514,13 @@
 	if (rename(from, to) < 0)
 		return -1;
 
+
 	oldmask = umask(0);
 	if ((n = open(from, O_CREAT|O_RDWR, st.st_mode)) >= 0)
 		close(n);
 	umask(oldmask);
 
+
 	return 0;
 }
 
@@ -565,13 +544,13 @@
 	struct relay_request *r;
 	struct timeval tv;
 	struct relay_stats stats;
+	struct stat st;
 	fd_set readfds;
 	char work[1030];
-	time_t now, uptime, last_rename = 0;
-	int i, n;
+	time_t now, uptime;
+	int i, n, x;
 	int state = STATE_RUN;
 	int id;
-	long fpos;
 
 	strNcpy(work, r_args->detail, sizeof(work) - 6);
 	strcat(work, ".work");
@@ -607,19 +586,47 @@
 		if (got_sigterm) state = STATE_SHUTDOWN;
 
 		/*
-		 *	Open detail file - if needed, and if we can.
+		 *	Try to open our work file first.  If not, check if the
+		 *	actual detail file has stuff in it.  If so, open it,
+		 *	move it to the work file, and replace it with an empty
+		 *	file.  Then keep the file handle for locking/reading.
 		 */
 		if (state == STATE_RUN && fp == NULL) {
-			if ((fp = fopen(work, "r+")) != NULL)
-				state = STATE_BACKLOG;
-			else
-				fp = fopen(r_args->detail, "r+");
-			if (fp == NULL) {
-				fprintf(stderr, "%s: Unable to open detail file - %s\n", progname, r_args->detail);
-				perror("fopen");
-				return;
+			if ((fp = fopen(work, "r+")) == NULL) {
+				if(!stat(r_args->detail, &st)) {
+					if(st.st_size > 0) {
+						detail_move(r_args->detail, work);
+						continue;
+					}
+				}
 			}
 
+			/*
+			 * Try to lock the detail-file.
+			 * If lockf is used we want to lock the _whole_ file, hence the
+			 * fseek to the start of the file.
+			 * 
+			 * If we can't lock the detail file, we close it and
+			 * will try it again on the next iteration.  Note that
+			 * we will never unlock the file, on purpose, to stop
+			 * rlm_detail from reading it.
+			 */
+			
+			if(fp) {
+				fseek(fp, 0L, SEEK_SET);
+				do {
+					x = rad_lockfd_nonblock(fileno(fp), 0);
+					if (x == -1)
+						ms_sleep(100);
+				} while (x == -1 && i++ < 20);
+
+				if (x == -1) {
+					fclose(fp);
+					fp = NULL;
+				} else {
+					state = STATE_BACKLOG;
+				}
+			}
 		}
 
 		/*
@@ -640,43 +647,6 @@
 					state = STATE_CLOSE;
 					break;
 				}
-
-				/*
-				 *	End of file. See if the file has
-				 *	any size, and if we renamed less
-				 *	than 10 seconds ago or not.
-				 */
-				now = time(NULL);
-				if (ftell(fp) == 0 || now < last_rename + 10) {
-					fpos = ftell(fp);
-					fseek(fp, 0L, SEEK_SET);
-					rad_unlockfd(fileno(fp), 0);
-					fseek(fp, fpos, SEEK_SET);
-					break;
-				}
-				last_rename = now;
-
-				/*
-				 *	We rename the file to <file>.work
-				 *	and create an empty new file.
-				 */
-				if (detail_move(r_args->detail, work) == 0) {
-					if (debug_flag > 0)
-						fprintf(stderr, "Moving %s to %s\n",
-							r_args->detail, work);
-					/*
-					 *	rlm_detail might still write
-					 *	something to <detail>.work if
-					 *	it opens <detail> before it is
-					 *	renamed (race condition)
-					 */
-					ms_sleep(1000);
-					state = STATE_BACKLOG;
-				}
-				fpos = ftell(fp);
-				fseek(fp, 0L, SEEK_SET);
-				rad_unlockfd(fileno(fp), 0);
-				fseek(fp, fpos, SEEK_SET);
 			} while(0);
 			if (r_args->records_print && state == STATE_RUN){
 				stats.records_read++;

    