| Message ID | 20191107142613.2379-2-robert.foley@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make the qemu_logfile handle thread safe. | expand |
Robert Foley <robert.foley@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.foley@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
On Thu, 7 Nov 2019 at 11:53, Alex Bennée <alex.bennee@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.bennee@linaro.org> wrote: > > > Robert Foley <robert.foley@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.foley@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
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(); + } + 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 */
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.foley@linaro.org> --- util/log.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)