Re: [PATCH 1/4] Add a mutex to guarantee single writer to qemu_logfile handle.
On Thu, 7 Nov 2019 at 11:53, Alex Bennée 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 wrote: > > > Robert Foley 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 > > --- > > 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(_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(_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(_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
Re: [PATCH 1/4] Add a mutex to guarantee single writer to qemu_logfile handle.
Robert Foley 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 > --- > 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(_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(_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(_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
[PATCH 1/4] Add a mutex to guarantee single writer to qemu_logfile handle.
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 --- 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(_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(_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(_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 */ -- 2.17.1