On Mon, Oct 28, 2024 at 11:00:40AM +0100, Stefano Brivio wrote:We open the log file with O_APPEND, but switch it off before seeking, and turn it back on afterwards. We never seek when O_APPEND is on, so we don't actually need it, as its only function is to override the offset for writes so that they are always performed at the end regardless of the current offset (which is at the end anyway, for us).Sorry, this sounded fishy to me on the call, but I figured I was just missing something. But looking at this the reasoning doesn't make sense to me. We don't seek with O_APPEND, but we do write(), which is exactly where it matters. AIUI the point of O_APPEND is that if you have multiple processes writing to the same file, they won't clobber each others writes because of a stale file pointer. That's usually not _necessary_ for us as such, but it's perhaps valuable since it reduces the likelihood of data loss if somehow you do get two instances logging to the same file. Of course the rotation process *can* clobber things (which is exactly why I was always a bit sceptical of this "in place" rotation, not that we really have other options). But that at least is occasional, unlike each log write. Maybe it's not worth having O_APPEND, but I don't think the reasoning above makes any real sense.Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- log.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/log.c b/log.c index 6932885..dd25862 100644 --- a/log.c +++ b/log.c @@ -204,9 +204,6 @@ out: */ static int logfile_rotate(int fd, const struct timespec *now) { - if (fcntl(fd, F_SETFL, O_RDWR /* Drop O_APPEND: explicit lseek() */)) - return -errno; - #ifdef FALLOC_FL_COLLAPSE_RANGE /* Only for Linux >= 3.15, extent-based ext4 or XFS, glibc >= 2.18 */ if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size)) @@ -215,9 +212,6 @@ static int logfile_rotate(int fd, const struct timespec *now) #endif logfile_rotate_move(fd, now); - if (fcntl(fd, F_SETFL, O_RDWR | O_APPEND)) - return -errno; - return 0; } @@ -416,7 +410,7 @@ void logfile_init(const char *name, const char *path, size_t size) if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) die_perror("Failed to read own /proc/self/exe link"); - log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC, + log_file = open(path, O_CREAT | O_TRUNC | O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR); if (log_file == -1) die_perror("Couldn't open log file %s", path);-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson