MINOR: sink/ring: rotate non-empty file-backed contents only

If the service is rechecked before a reload, that may cause the config
to be parsed twice and file-backed rings to be lost.

Here we make sure that such a ring does contain information before
deciding to rotate it. This way the first process starting after some
writes will cause a rotate but not subsequent ones until new writes
are applied.

An attempt was also made to disable rotations on checks but this was a
bad idea, as the ring is still initialized and this causes the contents
to be lost. The choice of initializing the ring during parsing is
questionable but the config check ought to be as close as possible to a
real start, and we could imagine that the ring is used by some code
during startup (e.g. lua). So this approach was abandonned and config
checks also cause a rotation, as the purpose of this rotation is to
preserve latest information against accidental removal.

doc/configuration.txt

@@ -3603,12 +3603,18 @@ backing-file <path>
   the "struct ring" that starts at the beginning of the area, and that is
   required to recover the area's contents. The file will be created with the
   starting user's ownership, with mode 0600 and will be of the size configured
-  by the "size" directive. Any previously existing file will be renamed with
+  by the "size" directive. When the directive is parsed (thus even during
-  the extra suffix ".bak", and any previously existing file with suffix ".bak"
+  config checks), any existing non-empty file will first be renamed with the
-  will be removed. This ensures that instant restart of the process will not
+  extra suffix ".bak", and any previously existing file with suffix ".bak" will
-  wipe precious debugging information, and will leave time for an admin to spot
+  be removed. This ensures that instant reload or restart of the process will
-  this new ".bak" file and to archive it if needed. This means that the total
+  not wipe precious debugging information, and will leave time for an admin to
-  storage capacity required will be double of the ring size.
+  spot this new ".bak" file and to archive it if needed. As such, after a crash
+  the file designated by <path> will contain the freshest information, and if
+  the service is restarted, the "<path>.bak" file will have it instead. This
+  means that the total storage capacity required will be double of the ring
+  size. Failures to rotate the file are silently ignored, so placing the file
+  into a directory without write permissions will be sufficient to avoid the
+  backup file if not desired.
 
   WARNING: there are stability and security implications in using this feature.
   First, backing the ring to a slow device (e.g. physical hard drive) may cause

src/sink.c

@@ -751,6 +751,54 @@ int sink_init_forward(struct sink *sink)
 	task_wakeup(sink->forward_task, TASK_WOKEN_INIT);
 	return 1;
 }
+
+/* This tries to rotate a file-backed ring, but only if it contains contents.
+ * This way empty rings will not cause backups to be overwritten and it's safe
+ * to reload multiple times. That's only best effort, failures are silently
+ * ignored.
+ */
+void sink_rotate_file_backed_ring(const char *name)
+{
+	struct ring ring;
+	char *oldback;
+	int ret;
+	int fd;
+
+	fd = open(name, O_RDONLY);
+	if (fd < 0)
+		return;
+
+	/* check for contents validity */
+	ret = read(fd, &ring, sizeof(ring));
+	close(fd);
+
+	if (ret != sizeof(ring))
+		goto rotate;
+
+	/* contents are present, we want to keep them => rotate. Note that
+	 * an empty ring buffer has one byte (the marker).
+	 */
+	if (ring.buf.data > 1)
+		goto rotate;
+
+	/* nothing to keep, let's scratch the file and preserve the backup */
+	return;
+
+ rotate:
+	oldback = NULL;
+	memprintf(&oldback, "%s.bak", name);
+	if (oldback) {
+		/* try to rename any possibly existing ring file to
+		 * ".bak" and delete remains of older ones. This will
+		 * ensure we don't wipe useful debug info upon restart.
+		 */
+		unlink(oldback);
+		if (rename(name, oldback) < 0)
+			unlink(oldback);
+		ha_free(&oldback);
+	}
+}
+
 /*
  * Parse "ring" section and create corresponding sink buffer.
  *
@@ -846,7 +894,6 @@ int cfg_parse_ring(const char *file, int linenum, char **args, int kwm)
 		 * for ring <ring>. Existing data are delete. NULL is returned on error.
 		 */
 		const char *backing = args[1];
-		char *oldback;
 		size_t size;
 		void *area;
 		int fd;
@@ -863,18 +910,12 @@ int cfg_parse_ring(const char *file, int linenum, char **args, int kwm)
 			goto err;
 		}
 
-		oldback = NULL;
+		/* let's check if the file exists and is not empty. That's the
-		memprintf(&oldback, "%s.bak", backing);
+		 * only condition under which we'll trigger a rotate, so that
-		if (oldback) {
+		 * config checks, reloads, or restarts that don't emit anything
-			/* try to rename any possibly existing ring file to
+		 * do not rotate it again.
-			 * ".bak" and delete remains of older ones. This will
+		 */
-			 * ensure we don't wipe useful debug info upon restart.
+		sink_rotate_file_backed_ring(backing);
-			 */
-			unlink(oldback);
-			if (rename(backing, oldback) < 0)
-				unlink(oldback);
-			ha_free(&oldback);
-		}
 
 		fd = open(backing, O_RDWR | O_CREAT, 0600);
 		if (fd < 0) {