From patchwork Sat Jan 29 19:38:16 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1586252 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4JmPky1q4jz9t8j for ; Sun, 30 Jan 2022 06:38:32 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 22D4260B2E; Sat, 29 Jan 2022 19:38:30 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8iq2JPlH4uuK; Sat, 29 Jan 2022 19:38:29 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp3.osuosl.org (Postfix) with ESMTPS id 32727605AD; Sat, 29 Jan 2022 19:38:28 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 08704C001A; Sat, 29 Jan 2022 19:38:28 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 2003AC000B for ; Sat, 29 Jan 2022 19:38:26 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id F0DC8605AD for ; Sat, 29 Jan 2022 19:38:25 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 33YZ7uGAM-kJ for ; Sat, 29 Jan 2022 19:38:25 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [217.70.178.230]) by smtp3.osuosl.org (Postfix) with ESMTPS id B5B19600D1 for ; Sat, 29 Jan 2022 19:38:24 +0000 (UTC) Received: (Authenticated sender: blp@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id 96FC6240005; Sat, 29 Jan 2022 19:38:20 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Sat, 29 Jan 2022 11:38:16 -0800 Message-Id: <20220129193816.1175611-1-blp@ovn.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Cc: Ben Pfaff , Antonin Bas Subject: [ovs-dev] [PATCH v2] daemon-unix: Close log file in monitor process while waiting on child. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" Until now, the monitor process held its log file open while it waited for the child to exit, and then it reopened it after the child exited. Holding the log file open this way prevented log rotation from freeing disk space. This commit avoids the problem by closing the log file before waiting, then reopening it afterward. Signed-off-by: Ben Pfaff Reported-by: Antonin Bas VMware-BZ: #2743409 --- v1->v2: Fix memory leak in monitor_daemon() reported by William. include/openvswitch/vlog.h | 2 + lib/daemon-unix.c | 5 +- lib/vlog.c | 93 ++++++++++++++++++++++++++------------ 3 files changed, 71 insertions(+), 29 deletions(-) diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h index 886fce5e0..e53ce6d81 100644 --- a/include/openvswitch/vlog.h +++ b/include/openvswitch/vlog.h @@ -131,7 +131,9 @@ void vlog_set_verbosity(const char *arg); /* Configuring log destinations. */ void vlog_set_pattern(enum vlog_destination, const char *pattern); +char *vlog_get_log_file(void); int vlog_set_log_file(const char *file_name); +void vlog_close_log_file(void); int vlog_reopen_log_file(void); #ifndef _WIN32 void vlog_change_owner_unix(uid_t, gid_t); diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index 34d45b82a..a02fd984b 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -360,12 +360,15 @@ monitor_daemon(pid_t daemon_pid) (unsigned long int) daemon_pid, status_msg); if (child_ready) { + char *log_file = vlog_get_log_file(); + vlog_close_log_file(); int error; do { retval = waitpid(daemon_pid, &status, 0); error = retval == -1 ? errno : 0; } while (error == EINTR); - vlog_reopen_log_file(); + vlog_set_log_file(log_file); + free(log_file); if (error) { VLOG_FATAL("waitpid failed (%s)", ovs_strerror(error)); } diff --git a/lib/vlog.c b/lib/vlog.c index 533f93755..0a615bb66 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -343,13 +343,25 @@ vlog_set_pattern(enum vlog_destination destination, const char *pattern) } } -/* Sets the name of the log file used by VLF_FILE to 'file_name', or to the - * default file name if 'file_name' is null. Returns 0 if successful, - * otherwise a positive errno value. */ -int -vlog_set_log_file(const char *file_name) +/* Returns a copy of the name of the log file used by VLF_FILE, or NULL if none + * is configured. The caller must eventually free the returned string. */ +char * +vlog_get_log_file(void) +{ + ovs_mutex_lock(&log_file_mutex); + char *fn = nullable_xstrdup(log_file_name); + ovs_mutex_unlock(&log_file_mutex); + + return fn; +} + +/* Sets the name of the log file used by VLF_FILE to 'new_log_file_name', or + * closes the current log file if 'new_log_file_name' is NULL. Takes ownership + * of 'new_log_file_name'. Returns 0 if successful, otherwise a positive errno + * value. */ +static int +vlog_set_log_file__(char *new_log_file_name) { - char *new_log_file_name; struct vlog_module *mp; struct stat old_stat; struct stat new_stat; @@ -358,25 +370,29 @@ vlog_set_log_file(const char *file_name) bool log_close; /* Open new log file. */ - new_log_file_name = (file_name - ? xstrdup(file_name) - : xasprintf("%s/%s.log", ovs_logdir(), program_name)); - new_log_fd = open(new_log_file_name, O_WRONLY | O_CREAT | O_APPEND, 0660); - if (new_log_fd < 0) { - VLOG_WARN("failed to open %s for logging: %s", - new_log_file_name, ovs_strerror(errno)); - free(new_log_file_name); - return errno; + if (new_log_file_name) { + new_log_fd = open(new_log_file_name, O_WRONLY | O_CREAT | O_APPEND, + 0660); + if (new_log_fd < 0) { + VLOG_WARN("failed to open %s for logging: %s", + new_log_file_name, ovs_strerror(errno)); + free(new_log_file_name); + return errno; + } + } else { + new_log_fd = -1; } /* If the new log file is the same one we already have open, bail out. */ ovs_mutex_lock(&log_file_mutex); - same_file = (log_fd >= 0 - && new_log_fd >= 0 - && !fstat(log_fd, &old_stat) - && !fstat(new_log_fd, &new_stat) - && old_stat.st_dev == new_stat.st_dev - && old_stat.st_ino == new_stat.st_ino); + same_file = ((log_fd < 0 + && new_log_fd < 0) || + (log_fd >= 0 + && new_log_fd >= 0 + && !fstat(log_fd, &old_stat) + && !fstat(new_log_fd, &new_stat) + && old_stat.st_dev == new_stat.st_dev + && old_stat.st_ino == new_stat.st_ino)); ovs_mutex_unlock(&log_file_mutex); if (same_file) { close(new_log_fd); @@ -392,19 +408,18 @@ vlog_set_log_file(const char *file_name) VLOG_INFO("closing log file"); } - /* Close old log file, if any, and install new one. */ + /* Close old log file, if any. */ ovs_mutex_lock(&log_file_mutex); if (log_fd >= 0) { close(log_fd); - async_append_destroy(log_writer); } - + async_append_destroy(log_writer); free(log_file_name); - log_file_name = xstrdup(new_log_file_name); + + /* Install new log file. */ + log_file_name = nullable_xstrdup(new_log_file_name); log_fd = new_log_fd; - if (log_async) { - log_writer = async_append_create(new_log_fd); - } + log_writer = log_async ? async_append_create(new_log_fd) : NULL; LIST_FOR_EACH (mp, list, &vlog_modules) { update_min_level(mp); @@ -418,6 +433,28 @@ vlog_set_log_file(const char *file_name) return 0; } +/* Closes the log file, if any. + * + * If the real goal is open a new log file, use vlog_set_log_file() to directly + * do that: there is no need to close the old file first. */ +void +vlog_close_log_file(void) +{ + vlog_set_log_file__(NULL); +} + +/* Sets the name of the log file used by VLF_FILE to 'file_name', or to the + * default file name if 'file_name' is null. Returns 0 if successful, + * otherwise a positive errno value. */ +int +vlog_set_log_file(const char *file_name) +{ + return vlog_set_log_file__( + file_name + ? xstrdup(file_name) + : xasprintf("%s/%s.log", ovs_logdir(), program_name)); +} + /* Closes and then attempts to re-open the current log file. (This is useful * just after log rotation, to ensure that the new log file starts being used.) * Returns 0 if successful, otherwise a positive errno value. */