util/log.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
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(-)
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 */
--
2.37.3On 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 > > > >
© 2016 - 2024 Red Hat, Inc.