Re: [ovs-dev] [PATCH] daemon-unix: Close log file in monitor process while waiting on child.
On Wed, Jan 26, 2022 at 06:39:52PM -0800, William Tu wrote: > On Wed, Jan 26, 2022 at 8:56 AM Ben Pfaff wrote: > > > > 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 > > --- > > Hi Ben, > Thanks, ASAN reports a memory leak, [...] > I think we should "free(log_file)" after vlog_set_log_file(log_file)? You are right. Thank you for the report. I posted v2: https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/391119.html ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] daemon-unix: Close log file in monitor process while waiting on child.
On Wed, Jan 26, 2022 at 8:56 AM Ben Pfaff wrote:
>
> 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
> ---
Hi Ben,
Thanks, ASAN reports a memory leak,
2022-01-27T02:17:41.3146557Z 1788: ovsdb-server/add-db with --monitor
FAILED (ovs-macros.at:217)
2022-01-27T02:17:41.3467168Z 1789: ovsdb-server/add-db and remove-db
with --monitor FAILED (ovs-macros.at:217)
==29645==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 99 byte(s) in 1 object(s) allocated from:
#0 0x49633d in malloc
(/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/ovsdb/ovsdb-server+0x49633d)
#1 0x56df34 in xmalloc__
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/util.c:137:15
#2 0x56e098 in xmemdup0
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/util.c:193:15
#3 0x576800 in vlog_get_log_file
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/vlog.c:352:16
#4 0x57a80e in monitor_daemon
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/daemon-unix.c:363:30
#5 0x57a05a in daemonize_start
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/daemon-unix.c:481:13
#6 0x4c5e66 in main
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ovsdb/ovsdb-server.c:356:5
#7 0x7f0effa20bf6 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
SUMMARY: AddressSanitizer: 99 byte(s) leaked in 1 allocation(s).
I think we should "free(log_file)" after vlog_set_log_file(log_file)?
William
> include/openvswitch/vlog.h | 2 +
> lib/daemon-unix.c | 4 +-
> lib/vlog.c | 93 ++
> 3 files changed, 70 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..10806bf81 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -360,12 +360,14 @@ 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, , 0);
> error = retval == -1 ? errno : 0;
> } while (error == EINTR);
> -vlog_reopen_log_file();
> +vlog_set_log_file(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(_file_mutex);
> +char *fn = nullable_xstrdup(log_file_name);
> +ovs_mutex_unlock(_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)
> - :
[ovs-dev] [PATCH] daemon-unix: Close log file in monitor process while waiting on child.
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
---
include/openvswitch/vlog.h | 2 +
lib/daemon-unix.c | 4 +-
lib/vlog.c | 93 ++
3 files changed, 70 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..10806bf81 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -360,12 +360,14 @@ 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, , 0);
error = retval == -1 ? errno : 0;
} while (error == EINTR);
-vlog_reopen_log_file();
+vlog_set_log_file(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(_file_mutex);
+char *fn = nullable_xstrdup(log_file_name);
+ovs_mutex_unlock(_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(_file_mutex);
-same_file = (log_fd >= 0
- && new_log_fd >= 0
- && !fstat(log_fd, _stat)
- && !fstat(new_log_fd, _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, _stat)
+ && !fstat(new_log_fd, _stat)
+ && old_stat.st_dev == new_stat.st_dev
+ && old_stat.st_ino == new_stat.st_ino));
ovs_mutex_unlock(_file_mutex);
if (same_file) {
close(new_log_fd);
@@ -392,19 +408,18 @@ vlog_set_log_file(const char *file_name)
