On Thu, 7 Nov 2019 at 11:53, Alex Bennée <alex.ben...@linaro.org> wrote: > > It wouldn't be the worst thing in the world to expose: > > qemu_logfile_init() > > and make vl.c and main.c call it before the setup. Then you can drop the > flag or even just g_assert(qemu_log_mutex_initialised) in qemu_set_log > and qemu_set_logfile. > > In fact you could just use: > > static void __attribute__((__constructor__)) qemu_logfile_init(void) > > and make the compiler do it for you.
All good ideas. Will make the changes. I agree, it is much cleaner to call init this way (constructor). We can assert that qemu_log_mutex.initialized on use of the mutex (qemu_set_log and qemu_set_logfile). Taking that one step further, we could add simple helper functions for qemu_logfile_mutex_lock()/unlock(), which g_assert() on mutex.initialized first before lock/unlock. Thanks, -Rob On Thu, 7 Nov 2019 at 11:53, Alex Bennée <alex.ben...@linaro.org> wrote: > > > Robert Foley <robert.fo...@linaro.org> writes: > > > This is being added in preparation for using RCU with the logfile handle. > > Also added qemu_logfile_init() for initializing the logfile mutex. > > > > Signed-off-by: Robert Foley <robert.fo...@linaro.org> > > --- > > util/log.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/util/log.c b/util/log.c > > index 1ca13059ee..dff2f98c8c 100644 > > --- a/util/log.c > > +++ b/util/log.c > > @@ -24,8 +24,11 @@ > > #include "qapi/error.h" > > #include "qemu/cutils.h" > > #include "trace/control.h" > > +#include "qemu/thread.h" > > > > static char *logfilename; > > +static bool qemu_logfile_initialized; > > +static QemuMutex qemu_logfile_mutex; > > FILE *qemu_logfile; > > int qemu_loglevel; > > static int log_append = 0; > > @@ -49,6 +52,14 @@ int qemu_log(const char *fmt, ...) > > return ret; > > } > > > > +static void qemu_logfile_init(void) > > +{ > > + if (!qemu_logfile_initialized) { > > + qemu_mutex_init(&qemu_logfile_mutex); > > + qemu_logfile_initialized = true; > > + } > > +} > > + > > static bool log_uses_own_buffers; > > > > /* enable or disable low levels log */ > > @@ -58,6 +69,12 @@ void qemu_set_log(int log_flags) > > #ifdef CONFIG_TRACE_LOG > > qemu_loglevel |= LOG_TRACE; > > #endif > > + > > + /* Is there a better place to call this to init the logfile subsystem? > > */ > > + if (!qemu_logfile_initialized) { > > + qemu_logfile_init(); > > + } > > It wouldn't be the worst thing in the world to expose: > > qemu_logfile_init() > > and make vl.c and main.c call it before the setup. Then you can drop the > flag or even just g_assert(qemu_log_mutex_initialised) in qemu_set_log > and qemu_set_logfile. > > In fact you could just use: > > static void __attribute__((__constructor__)) qemu_logfile_init(void) > > and make the compiler do it for you. > > > + qemu_mutex_lock(&qemu_logfile_mutex); > > if (!qemu_logfile && > > (is_daemonized() ? logfilename != NULL : qemu_loglevel)) { > > if (logfilename) { > > @@ -93,6 +110,7 @@ void qemu_set_log(int log_flags) > > log_append = 1; > > } > > } > > + qemu_mutex_unlock(&qemu_logfile_mutex); > > if (qemu_logfile && > > (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) { > > qemu_log_close(); > > @@ -114,6 +132,11 @@ void qemu_set_log_filename(const char *filename, Error > > **errp) > > char *pidstr; > > g_free(logfilename); > > > > + /* Is there a better place to call this to init the logfile subsystem? > > */ > > + if (!qemu_logfile_initialized) { > > + qemu_logfile_init(); > > + } > > + > > pidstr = strstr(filename, "%"); > > if (pidstr) { > > /* We only accept one %d, no other format strings */ > > > -- > Alex Bennée