On Wed, Jan 26, 2022 at 8:56 AM Ben Pfaff <b...@ovn.org> 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 <b...@ovn.org>
> Reported-by: Antonin Bas <a...@vmware.com>
> 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, &status, 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(&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. */
> --
> 2.33.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
