From 32872db6050cf39bf8ba80e40a2b6c4ee184a8e6 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 31 Aug 2022 18:52:17 +0200 Subject: [PATCH] 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 | 18 ++++++++---- src/sink.c | 67 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 66 insertions(+), 19 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index 24dc9e39d6..d0fc74f9e3 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -3603,12 +3603,18 @@ backing-file 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 - the extra suffix ".bak", and any previously existing file with suffix ".bak" - will be removed. This ensures that instant restart of the process will not - wipe precious debugging information, and will leave time for an admin to spot - this new ".bak" file and to archive it if needed. This means that the total - storage capacity required will be double of the ring size. + by the "size" directive. When the directive is parsed (thus even during + config checks), any existing non-empty file will first be renamed with the + extra suffix ".bak", and any previously existing file with suffix ".bak" will + be removed. This ensures that instant reload or restart of the process will + not wipe precious debugging information, and will leave time for an admin to + spot this new ".bak" file and to archive it if needed. As such, after a crash + the file designated by will contain the freshest information, and if + the service is restarted, the ".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 diff --git a/src/sink.c b/src/sink.c index 806eb1706c..2eb050a16d 100644 --- a/src/sink.c +++ b/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 . 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; - memprintf(&oldback, "%s.bak", backing); - 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(backing, oldback) < 0) - unlink(oldback); - ha_free(&oldback); - } + /* let's check if the file exists and is not empty. That's the + * only condition under which we'll trigger a rotate, so that + * config checks, reloads, or restarts that don't emit anything + * do not rotate it again. + */ + sink_rotate_file_backed_ring(backing); fd = open(backing, O_RDWR | O_CREAT, 0600); if (fd < 0) {