frontends/rnews.c: fix locking (this time correctly)

Miquel van Smoorenburg miquels at cistron.nl
Tue Jan 21 10:44:47 UTC 2003



Locking of multiple rnews -U queue runners is still not correct,
due to the stupid semantics of POSIX locking. As soon as any
filedescriptor on a file is close()d, even if it was open()ed
seperately, all locks on that file are lost.

This means you cannot use fdopen() in any way unless you simply
do not call fclose(), or delay it. That's becoming too complicated,
so I just rewrote ReadRemainder() in rnews.c to not use stdio at all.

Also to get an exclusive lock on a file it needs to be opened
in O_RDWR mode. All those fixes are in the patch below. Has been
tested on a busy machine with strace's running on several
rnews -U processes to check correctness.

--- inn-2.4-20030113.orig/frontends/rnews.c	2003-01-13 13:23:35.000000000 +0100
+++ inn-2.4-20030113/frontends/rnews.c	2003-01-21 11:36:44.000000000 +0100
@@ -333,24 +333,20 @@
 
 
 /*
-**  Read the rest of the input as an article.  Just punt to stdio in
-**  this case and let it do the buffering.
+**  Read the rest of the input as an article.
 */
 static bool
 ReadRemainder(int fd, char first, char second)
 {
-    FILE	*F;
     char	*article;
-    char       *p;
-    int	size;
-    int	used;
-    int	left;
-    int	i;
-    bool		ok;
-
-    /* Turn the descriptor into a stream. */
-    if ((i = dup(fd)) < 0 || (F = fdopen(i, "r")) == NULL)
-        sysdie("cannot fdopen %d", fd);
+    char	*p;
+    char	buf[BUFSIZ];
+    int		size;
+    int		used;
+    int		left;
+    int		skipnl;
+    int		i, n;
+    bool	ok;
 
     /* Get an initial allocation, leaving space for the \0. */
     size = BUFSIZ + 1;
@@ -359,32 +355,37 @@
     article[1] = second;
     used = second ? 2 : 1;
     left = size - used;
+    skipnl = 0;
 
-    /* Read the input.  Read a line at a time so that we can convert line
-       endings if necessary. */
-    p = fgets(article + used, left, F);
-    while (p != NULL) {
-	i = strlen(p);
-        if (p[i-2] == '\r') {
-            p[i-2] = '\n';
-            p[i-1] = '\0';
-        }
-	used += i;
-        left -= i;
-        if (left < SMBUF) {
-            size += BUFSIZ;
-            left += BUFSIZ;
-            RENEW(article, char, size);
-        }
-        p = fgets(article + used, left, F);
+    /* Read the input, coverting line ends as we go if necessary. */
+    while ((n = read(fd, buf, sizeof(buf))) > 0) {
+	p = article + used;
+	for (i = 0; i < n; i++) {
+	    if (skipnl) {
+		skipnl = 0;
+		if (buf[i] == '\n') continue;
+	    }
+	    if (buf[i] == '\r') {
+		buf[i] = '\n';
+		skipnl = 1;
+	    }
+	    *p++ = buf[i];
+	    used++;
+	    left--;
+	    if (left < SMBUF) {
+		size += BUFSIZ;
+		left += BUFSIZ;
+		RENEW(article, char, size);
+		p = article + used;
+	    }
+	}
     }
-    if (!feof(F))
-        sysdie("cannot fgets after %d bytes", used);
+    if (n < 0)
+	sysdie("cannot read after %d bytes", used);
 
     if (article[used - 1] != '\n')
 	article[used++] = '\n';
     article[used] = '\0';
-    fclose(F);
 
     ok = Process(article);
     DISPOSE(article);
@@ -609,7 +610,7 @@
     bool	ok;
     struct stat		Sb;
     char		hostname[10];
-    int			fd, lockfd;
+    int			fd, fd2;
     size_t		i;
     char                *badname, *uuhost;
 
@@ -635,26 +636,14 @@
 	if (!S_ISREG(Sb.st_mode))
 	    continue;
 
-	if ((fd = open(InputFile, O_RDONLY)) < 0) {
+	if ((fd = open(InputFile, O_RDWR)) < 0) {
 	    if (errno != ENOENT)
                 syswarn("cannot open %s", InputFile);
 	    continue;
 	}
 
-	/*
-	** Make sure multiple Unspools don't stomp on eachother.
-	** Because of stupid POSIX locking semantics, we need to lock
-	** on a seperate fd. Otherwise, dup()ing and then close()ing
-	** the dup()ed fd removes the lock we're holding (sigh).
-	*/
-	if ((lockfd = open(InputFile, O_RDONLY)) < 0) {
-	    if (errno != ENOENT)
-                syswarn("cannot open %s", InputFile);
-	    close(fd);
-	    continue;
-	}
-	if (!inn_lock_file(lockfd, INN_LOCK_READ, 0)) {
-	    close(lockfd);
+	/* Make sure multiple Unspools don't stomp on eachother. */
+	if (!inn_lock_file(fd, INN_LOCK_WRITE, 0)) {
 	    close(fd);
 	    continue;
 	}
@@ -677,22 +666,20 @@
 
 	if (!ok) {
             badname = concat(PathBadNews, "/", hostname, "XXXXXX", (char *) 0);
-            fd = mkstemp(badname);
-            if (fd < 0)
+            fd2 = mkstemp(badname);
+            if (fd2 < 0)
                 sysdie("cannot create temporary file");
-            close(fd);
+            close(fd2);
             warn("cant unspool saving to %s", badname);
 	    if (rename(InputFile, badname) < 0)
                 sysdie("cannot rename %s to %s", InputFile, badname);
 	    close(fd);
-	    close(lockfd);
 	    continue;
 	}
 
 	if (unlink(InputFile) < 0)
             syswarn("cannot remove %s", InputFile);
 	close(fd);
-	close(lockfd);
     }
     closedir(dp);
 


More information about the inn-patches mailing list