| Message ID | 20221014060807.660587-1-groug@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | util/log: Always send errors to logfile when daemonized | expand |
On 10/14/22 08:08, Greg Kurz wrote: > > + need_to_open_file = log_flags && !per_thread; Pre-existing, but I think this should check log_per_thread instead of per_thread. > + } else if (filename) { > + /* > + * If we are daemonized, we will only log if there is a filename. > + */ > + need_to_open_file = true; Slightly nicer: } else { /* * If daemonized, always log to the -D file if present. */ need_to_open_file = filename != NULL; } > @@ -271,10 +276,22 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name, > > if (!logfile && need_to_open_file) { > if (filename) { > - logfile = fopen(filename, log_append ? "a" : "w"); > + g_autofree char *fname = NULL; > + > + /* > + * If per-thread, filename contains a single %d that should be > + * converted. > + */ > + if (per_thread) { > + fname = g_strdup_printf(filename, getpid()); > + } else { > + fname = g_strdup(filename); > + } > + > + logfile = fopen(fname, log_append ? "a" : "w"); > if (!logfile) { > error_setg_errno(errp, errno, "Error opening logfile %s", > - filename); > + fname); > return false; > } > /* In case we are a daemon redirect stderr to logfile */ This could conflict with the file opened by qemu_log_trylock() when per-thread logging is enabled *and* QEMU is daemonized. Perhaps something like: 1) change qemu_log_trylock() to - if (log_per_thread) { + if (log_per_thread && log_thread_id() != getpid()) { i.e. use the global_file for the main thread 2) change qemu_log_unlock() to - if (!log_per_thread) { + if (!thread_file) { to match (1) 3) change log_thread_id() to something like ... #else static __thread int my_id = -1; static int counter; if (my_id == -1) { my_id = getpid() + qatomic_fetch_inc(&counter); } return my_id; #endif and perhaps do a dummy trylock/unlock late in qemu_set_log_internal(), to ensure that the main thread is the one with log_thread_id() == getpid()? I think this can be a separate patch before this one. Paolo
On Fri, 14 Oct 2022 10:51:36 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 10/14/22 08:08, Greg Kurz wrote: > > > > + need_to_open_file = log_flags && !per_thread; > > Pre-existing, but I think this should check log_per_thread instead of > per_thread. > Yes I agree, and also check qemu_loglevel instead of log_flags for the same reason (and to match the comment just above). > > + } else if (filename) { > > + /* > > + * If we are daemonized, we will only log if there is a filename. > > + */ > > + need_to_open_file = true; > > Slightly nicer: > > } else { > /* > * If daemonized, always log to the -D file if present. > */ > need_to_open_file = filename != NULL; > } > Sure. > > @@ -271,10 +276,22 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name, > > > > if (!logfile && need_to_open_file) { > > if (filename) { > > - logfile = fopen(filename, log_append ? "a" : "w"); > > + g_autofree char *fname = NULL; > > + > > + /* > > + * If per-thread, filename contains a single %d that should be > > + * converted. > > + */ > > + if (per_thread) { > > + fname = g_strdup_printf(filename, getpid()); > > + } else { > > + fname = g_strdup(filename); > > + } > > + > > + logfile = fopen(fname, log_append ? "a" : "w"); > > if (!logfile) { > > error_setg_errno(errp, errno, "Error opening logfile %s", > > - filename); > > + fname); > > return false; > > } > > /* In case we are a daemon redirect stderr to logfile */ > > This could conflict with the file opened by qemu_log_trylock() when > per-thread logging is enabled *and* QEMU is daemonized. Perhaps > something like: > Yeah... if the main thread happens to call qemu_log(), it then opens a file with the same name indeed. Thanks for catching that ! > 1) change qemu_log_trylock() to > > - if (log_per_thread) { > + if (log_per_thread && log_thread_id() != getpid()) { > > i.e. use the global_file for the main thread > > 2) change qemu_log_unlock() to > > - if (!log_per_thread) { > + if (!thread_file) { > > to match (1) > > 3) change log_thread_id() to something like > > ... > #else > static __thread int my_id = -1; > static int counter; > if (my_id == -1) { > my_id = getpid() + qatomic_fetch_inc(&counter); > } > return my_id; > #endif > > and perhaps do a dummy trylock/unlock late in qemu_set_log_internal(), > to ensure that the main thread is the one with log_thread_id() == getpid()? > > I think this can be a separate patch before this one. > 2) and 3) can certainly be preparatory work but I think 1) should be squashed in my patch. Because of the !per_thread check in need_to_open_file, the existing code in qemu_set_log_internal() doesn't even open the global file and qemu_log_trylock() would always return NULL for the main thread. Thanks for the quick answer and suggestions ! > Paolo >
Il ven 14 ott 2022, 18:07 Greg Kurz <groug@kaod.org> ha scritto: > 2) and 3) can certainly be preparatory work but I think 1) > should be squashed in my patch. Because of the !per_thread > check in need_to_open_file, the existing code in > qemu_set_log_internal() doesn't even open the global file > and qemu_log_trylock() would always return NULL for the > main thread. > Sounds good, thanks! Paolo > Thanks for the quick answer and suggestions ! > > > Paolo > > > >
diff --git a/util/log.c b/util/log.c index d6eb0378c3a3..a4592fa9bb70 100644 --- a/util/log.c +++ b/util/log.c @@ -248,16 +248,21 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name, #endif qemu_loglevel = log_flags; - /* - * In all cases we only log if qemu_loglevel is set. - * Also: - * If per-thread, open the file for each thread in qemu_log_lock. - * If not daemonized we will always log either to stderr - * or to a file (if there is a filename). - * If we are daemonized, we will only log if there is a filename. - */ daemonized = is_daemonized(); - need_to_open_file = log_flags && !per_thread && (!daemonized || filename); + need_to_open_file = false; + if (!daemonized) { + /* + * If not daemonized we only log if qemu_loglevel is set, either to + * stderr or to a file (if there is a filename). + * If per-thread, open the file for each thread in qemu_log_trylock(). + */ + need_to_open_file = log_flags && !per_thread; + } else if (filename) { + /* + * If we are daemonized, we will only log if there is a filename. + */ + need_to_open_file = true; + } if (logfile && (!need_to_open_file || changed_name)) { qatomic_rcu_set(&global_file, NULL); @@ -271,10 +276,22 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name, if (!logfile && need_to_open_file) { if (filename) { - logfile = fopen(filename, log_append ? "a" : "w"); + g_autofree char *fname = NULL; + + /* + * If per-thread, filename contains a single %d that should be + * converted. + */ + if (per_thread) { + fname = g_strdup_printf(filename, getpid()); + } else { + fname = g_strdup(filename); + } + + logfile = fopen(fname, log_append ? "a" : "w"); if (!logfile) { error_setg_errno(errp, errno, "Error opening logfile %s", - filename); + fname); return false; } /* In case we are a daemon redirect stderr to logfile */
When QEMU is started with `-daemonize`, all stdio descriptors get redirected to `/dev/null`. This basically means that anything printed with `error_report()` and friends is lost. One could hope that passing `-D ${logfile}` would cause the messages to go to `${logfile}`, as the documentation tends to suggest: -D logfile Output log in logfile instead of to stderr Unfortunately, `-D` belongs to the logging framework and it only does this redirection if some log item is also enabled with `-d` or if QEMU was configured with `--enable-trace-backend=log`. A typical production setup doesn't do tracing or fine-grain debugging but it certainly needs to collect errors. Ignore the check on enabled log items when QEMU is daemonized. Previous behaviour is retained for the non-daemonized case. The logic is unrolled as a series of `if` for better readability. Special care is needed for the `-D ${logfile} -d tid` case : `${logfile}` is expected to be a template that contains exactly one `%d` that should be expanded to a PID or TID. The logic in `qemu_log_trylock()` already takes care of that for per-thread logs. Do it as well for the QEMU main thread when opening the file. Signed-off-by: Greg Kurz <groug@kaod.org> --- util/log.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)