| Message ID | 20191107142613.2379-3-robert.foley@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make the qemu_logfile handle thread safe. | expand |
Robert Foley <robert.foley@linaro.org> writes: > This now allows changing the logfile while logging is active, > and also solves the issue of a seg fault while changing the logfile. > > Any read access to the qemu_logfile handle will use > the rcu_read_lock()/unlock() around the use of the handle. > To fetch the handle we will use atomic_rcu_read(). > We also in many cases do a check for validity of the > logfile handle before using it to deal with the case where the > file is closed and set to NULL. > > The cases where we write to the qemu_logfile will use atomic_rcu_set(). > Writers will also use call_rcu() with a newly added qemu_logfile_free > function for freeing/closing when readers have finished. > > Signed-off-by: Robert Foley <robert.foley@linaro.org> > --- > include/qemu/log.h | 47 ++++++++++++++++++++++++---- > util/log.c | 78 ++++++++++++++++++++++++++++++++++------------ > include/exec/log.h | 33 +++++++++++++++++--- > tcg/tcg.c | 12 +++++-- > 4 files changed, 138 insertions(+), 32 deletions(-) > > diff --git a/include/qemu/log.h b/include/qemu/log.h > index a91105b2ad..975de18e23 100644 > --- a/include/qemu/log.h > +++ b/include/qemu/log.h > @@ -3,9 +3,17 @@ > > /* A small part of this API is split into its own header */ > #include "qemu/log-for-trace.h" > +#include "qemu/rcu.h" > + > +struct QemuLogFile { > + struct rcu_head rcu; > + FILE *fd; > +}; > +typedef struct QemuLogFile QemuLogFile; If you are declaring a typedef then do it in one: typedef struct QemuLogFile { ... We only really use the second form for opaque structs where the handle is passed around but the contents hidden from the rest of QEMU. > > /* Private global variable, don't use */ > -extern FILE *qemu_logfile; > +extern QemuLogFile *qemu_logfile; > + Do we need multiple > > /* > * The new API: > @@ -25,7 +33,17 @@ static inline bool qemu_log_enabled(void) > */ > static inline bool qemu_log_separate(void) > { > - return qemu_logfile != NULL && qemu_logfile != stderr; > + QemuLogFile *logfile; > + > + if (qemu_log_enabled()) { > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile && logfile->fd != stderr) { > + return true; > + } > + rcu_read_unlock(); > + } > + return false; This is unbalanced as you'll have incremented the reader count. Also qemu_log_enabled() is also synonymous with logfile existing so you could fold that into: bool res = false; rcu_read_lock(); *logfile = atomic_rcu_read(&qemu_logfile); if (logfile && logfile->fd != stderr) { res = true; } rcu_read_unlock(); return res; There is technically a race there as the answer may become invalid after qemu_log_separate() returns. However given the users I don't see what could fail afterwards. > } > > #define CPU_LOG_TB_OUT_ASM (1 << 0) > @@ -55,12 +73,23 @@ static inline bool qemu_log_separate(void) > > static inline void qemu_log_lock(void) > { > - qemu_flockfile(qemu_logfile); > + QemuLogFile *logfile; > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + qemu_flockfile(logfile->fd); > + } > + rcu_read_unlock(); > } static inline FILE *fd qemu_log_lock(void) { FILE *fd; rcu_read_lock(); fd = atomic_rcu_read(&qemu_logfile); if (fd) { qemu_flockfile(fd); return fd; } else { rcu_read_unlock(); return NULL; } } static inline qemu_log_unlock(FILE *fd) { if (fd) { qemu_funlockfile(fd); rcu_read_unlock(); } } While the rcu_read_lock() is in progress then we won't ever finish with the fd - which we don't want to do until the file locking is finished. <snip> > > /* Logging functions: */ > @@ -70,9 +99,14 @@ static inline void qemu_log_unlock(void) > static inline void GCC_FMT_ATTR(1, 0) > qemu_log_vprintf(const char *fmt, va_list va) > { > - if (qemu_logfile) { > - vfprintf(qemu_logfile, fmt, va); > + QemuLogFile *logfile; > + > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + vfprintf(logfile->fd, fmt, va); > } > + rcu_read_unlock(); > } > > /* log only if a bit is set on the current loglevel mask: > @@ -129,5 +163,6 @@ void qemu_print_log_usage(FILE *f); > void qemu_log_flush(void); > /* Close the log file */ > void qemu_log_close(void); > +void qemu_logfile_free(QemuLogFile *logfile); > > #endif > diff --git a/util/log.c b/util/log.c > index dff2f98c8c..d409532b8f 100644 > --- a/util/log.c > +++ b/util/log.c > @@ -29,7 +29,7 @@ > static char *logfilename; > static bool qemu_logfile_initialized; > static QemuMutex qemu_logfile_mutex; > -FILE *qemu_logfile; > +QemuLogFile *qemu_logfile; > int qemu_loglevel; > static int log_append = 0; > static GArray *debug_regions; > @@ -38,10 +38,14 @@ static GArray *debug_regions; > int qemu_log(const char *fmt, ...) > { > int ret = 0; > - if (qemu_logfile) { > + QemuLogFile *logfile; > + > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > va_list ap; > va_start(ap, fmt); > - ret = vfprintf(qemu_logfile, fmt, ap); > + ret = vfprintf(logfile->fd, fmt, ap); > va_end(ap); > > /* Don't pass back error results. */ > @@ -49,6 +53,7 @@ int qemu_log(const char *fmt, ...) > ret = 0; > } > } > + rcu_read_unlock(); > return ret; > } > > @@ -65,6 +70,8 @@ static bool log_uses_own_buffers; > /* enable or disable low levels log */ > void qemu_set_log(int log_flags) > { > + QemuLogFile *logfile; > + > qemu_loglevel = log_flags; > #ifdef CONFIG_TRACE_LOG > qemu_loglevel |= LOG_TRACE; > @@ -77,43 +84,51 @@ void qemu_set_log(int log_flags) > qemu_mutex_lock(&qemu_logfile_mutex); > if (!qemu_logfile && > (is_daemonized() ? logfilename != NULL : qemu_loglevel)) { > + logfile = g_new0(QemuLogFile, 1); > if (logfilename) { You can assume logfilename exists as glib memory allocations would abort() otherwise. > - qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); > - if (!qemu_logfile) { > + logfile->fd = fopen(logfilename, log_append ? "a" : "w"); > + if (!logfile->fd) { > + g_free(logfile); > perror(logfilename); > _exit(1); > } > /* In case we are a daemon redirect stderr to logfile */ > if (is_daemonized()) { > - dup2(fileno(qemu_logfile), STDERR_FILENO); > - fclose(qemu_logfile); > + dup2(fileno(logfile->fd), STDERR_FILENO); > + fclose(logfile->fd); > /* This will skip closing logfile in qemu_log_close() */ > - qemu_logfile = stderr; > + logfile->fd = stderr; > } > } else { > /* Default to stderr if no log file specified */ > assert(!is_daemonized()); > - qemu_logfile = stderr; > + logfile->fd = stderr; > } > /* must avoid mmap() usage of glibc by setting a buffer "by hand" */ > if (log_uses_own_buffers) { > static char logfile_buf[4096]; > > - setvbuf(qemu_logfile, logfile_buf, _IOLBF, sizeof(logfile_buf)); > + setvbuf(logfile->fd, logfile_buf, _IOLBF, sizeof(logfile_buf)); > } else { > #if defined(_WIN32) > /* Win32 doesn't support line-buffering, so use unbuffered output. */ > - setvbuf(qemu_logfile, NULL, _IONBF, 0); > + setvbuf(logfile->fd, NULL, _IONBF, 0); > #else > - setvbuf(qemu_logfile, NULL, _IOLBF, 0); > + setvbuf(logfile->fd, NULL, _IOLBF, 0); > #endif > log_append = 1; > } > + atomic_rcu_set(&qemu_logfile, logfile); > } > - qemu_mutex_unlock(&qemu_logfile_mutex); > + logfile = qemu_logfile; > + > if (qemu_logfile && > (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) { > - qemu_log_close(); > + atomic_rcu_set(&qemu_logfile, NULL); > + qemu_mutex_unlock(&qemu_logfile_mutex); > + call_rcu(logfile, qemu_logfile_free, rcu); > + } else { > + qemu_mutex_unlock(&qemu_logfile_mutex); > } > } > > @@ -247,17 +262,40 @@ out: > /* fflush() the log file */ > void qemu_log_flush(void) > { > - fflush(qemu_logfile); > + QemuLogFile *logfile; > + > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + fflush(logfile->fd); > + } > + rcu_read_unlock(); > +} > + > +void qemu_logfile_free(QemuLogFile *logfile) This can be static as it's internal to log.c > +{ > + if (logfile) { g_assert(logfile) instead of the if? > + if (logfile->fd != stderr) { > + fclose(logfile->fd); > + } > + g_free(logfile); > + } > } > > /* Close the log file */ > void qemu_log_close(void) > { > - if (qemu_logfile) { > - if (qemu_logfile != stderr) { > - fclose(qemu_logfile); > - } > - qemu_logfile = NULL; > + QemuLogFile *logfile; > + > + qemu_mutex_lock(&qemu_logfile_mutex); > + logfile = qemu_logfile; > + > + if (logfile) { > + atomic_rcu_set(&qemu_logfile, NULL); > + qemu_mutex_unlock(&qemu_logfile_mutex); I think you can move the both the unlocks out of the if/else and drop the else. > + call_rcu(logfile, qemu_logfile_free, rcu); > + } else { > + qemu_mutex_unlock(&qemu_logfile_mutex); > } > } > > diff --git a/include/exec/log.h b/include/exec/log.h > index e2cfd436e6..9bd1e4aa20 100644 > --- a/include/exec/log.h > +++ b/include/exec/log.h > @@ -15,8 +15,15 @@ > */ > static inline void log_cpu_state(CPUState *cpu, int flags) > { > + QemuLogFile *logfile; > + > if (qemu_log_enabled()) { > - cpu_dump_state(cpu, qemu_logfile, flags); > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + cpu_dump_state(cpu, logfile->fd, flags); > + } > + rcu_read_unlock(); > } > } > > @@ -40,19 +47,37 @@ static inline void log_cpu_state_mask(int mask, CPUState *cpu, int flags) > static inline void log_target_disas(CPUState *cpu, target_ulong start, > target_ulong len) > { > - target_disas(qemu_logfile, cpu, start, len); > + QemuLogFile *logfile; > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + target_disas(logfile->fd, cpu, start, len); > + } > + rcu_read_unlock(); > } > > static inline void log_disas(void *code, unsigned long size) > { > - disas(qemu_logfile, code, size); > + QemuLogFile *logfile; > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + disas(logfile->fd, code, size); > + } > + rcu_read_unlock(); > } > > #if defined(CONFIG_USER_ONLY) > /* page_dump() output to the log file: */ > static inline void log_page_dump(void) > { > - page_dump(qemu_logfile); > + QemuLogFile *logfile; > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); > + if (logfile) { > + page_dump(logfile->fd); > + } > + rcu_read_unlock(); > } > #endif > #endif > diff --git a/tcg/tcg.c b/tcg/tcg.c > index 5475d49ed1..220eaac7c7 100644 > --- a/tcg/tcg.c > +++ b/tcg/tcg.c > @@ -2114,9 +2114,17 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs) > } > > if (have_prefs || op->life) { > - for (; col < 40; ++col) { > - putc(' ', qemu_logfile); > + > + QemuLogFile *logfile; > + > + rcu_read_lock(); > + logfile = atomic_rcu_read(&qemu_logfile); This can probably be a qemu_log_lock() instead given interleaving output is going to be confusing. > + if (logfile) { > + for (; col < 40; ++col) { > + putc(' ', logfile->fd); > + } > } > + rcu_read_unlock(); and qemu_log_unlock(); This cleanup could even be in a separate patch. > } > > if (op->life) { -- Alex Bennée
On Thu, 7 Nov 2019 at 11:24, Alex Bennée <alex.bennee@linaro.org> wrote: > > Robert Foley <robert.foley@linaro.org> writes: <snip> > > diff --git a/include/qemu/log.h b/include/qemu/log.h > > index a91105b2ad..975de18e23 100644 > > --- a/include/qemu/log.h > > +++ b/include/qemu/log.h > > @@ -3,9 +3,17 @@ > > > > /* A small part of this API is split into its own header */ > > #include "qemu/log-for-trace.h" > > +#include "qemu/rcu.h" > > + > > +struct QemuLogFile { > > + struct rcu_head rcu; > > + FILE *fd; > > +}; > > +typedef struct QemuLogFile QemuLogFile; > > If you are declaring a typedef then do it in one: > > typedef struct QemuLogFile { ... > > We only really use the second form for opaque structs where the handle > is passed around but the contents hidden from the rest of QEMU. > OK, makes sense. Will correct it. Thanks for explaining. > > > > /* Private global variable, don't use */ > > -extern FILE *qemu_logfile; > > +extern QemuLogFile *qemu_logfile; > > + > > Do we need multiple Not 100% sure on the meaning here. Are you asking if we need to extern this? We have a few other cases outside of the log module where this is getting used so the extern is needed here. > > > > /* > > * The new API: > > @@ -25,7 +33,17 @@ static inline bool qemu_log_enabled(void) > > */ > > static inline bool qemu_log_separate(void) > > { > > - return qemu_logfile != NULL && qemu_logfile != stderr; > > + QemuLogFile *logfile; > > + > > + if (qemu_log_enabled()) { > > + rcu_read_lock(); > > + logfile = atomic_rcu_read(&qemu_logfile); > > + if (logfile && logfile->fd != stderr) { > > + return true; > > + } > > + rcu_read_unlock(); > > + } > > + return false; > > This is unbalanced as you'll have incremented the reader count. Also > qemu_log_enabled() is also synonymous with logfile existing so you could > fold that into: > > bool res = false; > > rcu_read_lock(); > *logfile = atomic_rcu_read(&qemu_logfile); > if (logfile && logfile->fd != stderr) { > res = true; > } > rcu_read_unlock(); > return res; > > There is technically a race there as the answer may become invalid after > qemu_log_separate() returns. However given the users I don't see what > could fail afterwards. > I agree, will make these changes. > > } > > > > #define CPU_LOG_TB_OUT_ASM (1 << 0) > > @@ -55,12 +73,23 @@ static inline bool qemu_log_separate(void) > > > > static inline void qemu_log_lock(void) > > { > > - qemu_flockfile(qemu_logfile); > > + QemuLogFile *logfile; > > + rcu_read_lock(); > > + logfile = atomic_rcu_read(&qemu_logfile); > > + if (logfile) { > > + qemu_flockfile(logfile->fd); > > + } > > + rcu_read_unlock(); > > } > > static inline FILE *fd qemu_log_lock(void) > { QemuLogFile *logfile; > rcu_read_lock(); logfile = atomic_rcu_read(&qemu_logfile); if (logfile) { > qemu_flockfile(logfile->fd); return logfile->fd; > } else { > rcu_read_unlock(); > return NULL; > } > } > > static inline qemu_log_unlock(FILE *fd) > { > if (fd) { > qemu_funlockfile(fd); > rcu_read_unlock(); > } > } > > While the rcu_read_lock() is in progress then we won't ever finish with > the fd - which we don't want to do until the file locking is finished. Agree with the adjustments you made to qemu_log_lock(). I updated the code above with a few tweaks for the QemuLogFile type returned by atomic_rcu_read(). Does this look OK or is there any other adjustment we should make here? The intent here was for qemu_log_lock() to hold the rcu_read_lock() until after we can qemu_funlockfile(fd). The idea being that we want to prevent the fclose(fd) from happening by holding the rcu_read_lock() across the qemu_log_lock() until qemu_log_unlock(). So we have the qemu_funlockfile(fd) first, and then once we're done with the fd, it is safe to rcu_read_unlock(). > <snip> <snip more> > > diff --git a/util/log.c b/util/log.c <snip> > > @@ -65,6 +70,8 @@ static bool log_uses_own_buffers; > > /* enable or disable low levels log */ > > void qemu_set_log(int log_flags) > > { > > + QemuLogFile *logfile; > > + > > qemu_loglevel = log_flags; > > #ifdef CONFIG_TRACE_LOG > > qemu_loglevel |= LOG_TRACE; > > @@ -77,43 +84,51 @@ void qemu_set_log(int log_flags) > > qemu_mutex_lock(&qemu_logfile_mutex); > > if (!qemu_logfile && > > (is_daemonized() ? logfilename != NULL : qemu_loglevel)) { > > + logfile = g_new0(QemuLogFile, 1); > > if (logfilename) { > > You can assume logfilename exists as glib memory allocations would > abort() otherwise. This is good to know about the glib memory allocations as it relates to the logfile above, since we did not add any check for null on allocation. We are thinking that logfilename might be NULL if we either never set it with a call to qemu_set_log_filename(), or (with our intended new fix) if we took an error actually opening the file in qemu_set_log_filename(), and went down the error path. > > - qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); > > - if (!qemu_logfile) { > > + logfile->fd = fopen(logfilename, log_append ? "a" : "w"); > > + if (!logfile->fd) { > > + g_free(logfile); > > perror(logfilename); > > _exit(1); <snip> > > +void qemu_logfile_free(QemuLogFile *logfile) > > This can be static as it's internal to log.c Absolutely, makes sense. Will change it. > > +{ > > + if (logfile) { > > g_assert(logfile) instead of the if? I agree, the assert is cleaner. Will change it. > > + if (logfile->fd != stderr) { > > + fclose(logfile->fd); > > + } > > + g_free(logfile); > > + } > > } > > > > /* Close the log file */ > > void qemu_log_close(void) > > { > > - if (qemu_logfile) { > > - if (qemu_logfile != stderr) { > > - fclose(qemu_logfile); > > - } > > - qemu_logfile = NULL; > > + QemuLogFile *logfile; > > + > > + qemu_mutex_lock(&qemu_logfile_mutex); > > + logfile = qemu_logfile; > > + > > + if (logfile) { > > + atomic_rcu_set(&qemu_logfile, NULL); > > + qemu_mutex_unlock(&qemu_logfile_mutex); > > I think you can move the both the unlocks out of the if/else and drop > the else. Agreed. Originally we were thinking to hold the lock until after the call_rcu, but I agree with you, it seems safe to move it. > > + call_rcu(logfile, qemu_logfile_free, rcu); > > + } else { > > + qemu_mutex_unlock(&qemu_logfile_mutex); > > } > > } <snip> > > diff --git a/tcg/tcg.c b/tcg/tcg.c > > index 5475d49ed1..220eaac7c7 100644 > > --- a/tcg/tcg.c > > +++ b/tcg/tcg.c > > @@ -2114,9 +2114,17 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs) > > } > > > > if (have_prefs || op->life) { > > - for (; col < 40; ++col) { > > - putc(' ', qemu_logfile); > > + > > + QemuLogFile *logfile; > > + > > + rcu_read_lock(); > > + logfile = atomic_rcu_read(&qemu_logfile); > > This can probably be a qemu_log_lock() instead given interleaving output > is going to be confusing. The function calling tcg_dump_ops(), tcg_gen_code(), already has the qemu_log_lock() held. We were thinking about another type of cleanup, but it applies to this case as well, since this case is using the qemu_logfile. We were thinking that at some point (another patch) it might be good to cleanup the use of the qemu_logfile global so that it is only referenced from inside the log module. We noticed a comment in log.h calling it a private global, which we thought might imply that it was an intent to keep it private, and that this cleanup might be a good direction to move in. Thanks, -Rob Foley > > + if (logfile) { > > + for (; col < 40; ++col) { > > + putc(' ', logfile->fd); > > + } > > } > > + rcu_read_unlock(); > > and qemu_log_unlock(); > > This cleanup could even be in a separate patch. > > > } > > > > if (op->life) { > > > -- > Alex Bennée
diff --git a/include/qemu/log.h b/include/qemu/log.h index a91105b2ad..975de18e23 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -3,9 +3,17 @@ /* A small part of this API is split into its own header */ #include "qemu/log-for-trace.h" +#include "qemu/rcu.h" + +struct QemuLogFile { + struct rcu_head rcu; + FILE *fd; +}; +typedef struct QemuLogFile QemuLogFile; /* Private global variable, don't use */ -extern FILE *qemu_logfile; +extern QemuLogFile *qemu_logfile; + /* * The new API: @@ -25,7 +33,17 @@ static inline bool qemu_log_enabled(void) */ static inline bool qemu_log_separate(void) { - return qemu_logfile != NULL && qemu_logfile != stderr; + QemuLogFile *logfile; + + if (qemu_log_enabled()) { + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile && logfile->fd != stderr) { + return true; + } + rcu_read_unlock(); + } + return false; } #define CPU_LOG_TB_OUT_ASM (1 << 0) @@ -55,12 +73,23 @@ static inline bool qemu_log_separate(void) static inline void qemu_log_lock(void) { - qemu_flockfile(qemu_logfile); + QemuLogFile *logfile; + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + qemu_flockfile(logfile->fd); + } + rcu_read_unlock(); } static inline void qemu_log_unlock(void) { - qemu_funlockfile(qemu_logfile); + QemuLogFile *logfile; + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + qemu_funlockfile(logfile->fd); + } + rcu_read_unlock(); } /* Logging functions: */ @@ -70,9 +99,14 @@ static inline void qemu_log_unlock(void) static inline void GCC_FMT_ATTR(1, 0) qemu_log_vprintf(const char *fmt, va_list va) { - if (qemu_logfile) { - vfprintf(qemu_logfile, fmt, va); + QemuLogFile *logfile; + + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + vfprintf(logfile->fd, fmt, va); } + rcu_read_unlock(); } /* log only if a bit is set on the current loglevel mask: @@ -129,5 +163,6 @@ void qemu_print_log_usage(FILE *f); void qemu_log_flush(void); /* Close the log file */ void qemu_log_close(void); +void qemu_logfile_free(QemuLogFile *logfile); #endif diff --git a/util/log.c b/util/log.c index dff2f98c8c..d409532b8f 100644 --- a/util/log.c +++ b/util/log.c @@ -29,7 +29,7 @@ static char *logfilename; static bool qemu_logfile_initialized; static QemuMutex qemu_logfile_mutex; -FILE *qemu_logfile; +QemuLogFile *qemu_logfile; int qemu_loglevel; static int log_append = 0; static GArray *debug_regions; @@ -38,10 +38,14 @@ static GArray *debug_regions; int qemu_log(const char *fmt, ...) { int ret = 0; - if (qemu_logfile) { + QemuLogFile *logfile; + + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { va_list ap; va_start(ap, fmt); - ret = vfprintf(qemu_logfile, fmt, ap); + ret = vfprintf(logfile->fd, fmt, ap); va_end(ap); /* Don't pass back error results. */ @@ -49,6 +53,7 @@ int qemu_log(const char *fmt, ...) ret = 0; } } + rcu_read_unlock(); return ret; } @@ -65,6 +70,8 @@ static bool log_uses_own_buffers; /* enable or disable low levels log */ void qemu_set_log(int log_flags) { + QemuLogFile *logfile; + qemu_loglevel = log_flags; #ifdef CONFIG_TRACE_LOG qemu_loglevel |= LOG_TRACE; @@ -77,43 +84,51 @@ void qemu_set_log(int log_flags) qemu_mutex_lock(&qemu_logfile_mutex); if (!qemu_logfile && (is_daemonized() ? logfilename != NULL : qemu_loglevel)) { + logfile = g_new0(QemuLogFile, 1); if (logfilename) { - qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); - if (!qemu_logfile) { + logfile->fd = fopen(logfilename, log_append ? "a" : "w"); + if (!logfile->fd) { + g_free(logfile); perror(logfilename); _exit(1); } /* In case we are a daemon redirect stderr to logfile */ if (is_daemonized()) { - dup2(fileno(qemu_logfile), STDERR_FILENO); - fclose(qemu_logfile); + dup2(fileno(logfile->fd), STDERR_FILENO); + fclose(logfile->fd); /* This will skip closing logfile in qemu_log_close() */ - qemu_logfile = stderr; + logfile->fd = stderr; } } else { /* Default to stderr if no log file specified */ assert(!is_daemonized()); - qemu_logfile = stderr; + logfile->fd = stderr; } /* must avoid mmap() usage of glibc by setting a buffer "by hand" */ if (log_uses_own_buffers) { static char logfile_buf[4096]; - setvbuf(qemu_logfile, logfile_buf, _IOLBF, sizeof(logfile_buf)); + setvbuf(logfile->fd, logfile_buf, _IOLBF, sizeof(logfile_buf)); } else { #if defined(_WIN32) /* Win32 doesn't support line-buffering, so use unbuffered output. */ - setvbuf(qemu_logfile, NULL, _IONBF, 0); + setvbuf(logfile->fd, NULL, _IONBF, 0); #else - setvbuf(qemu_logfile, NULL, _IOLBF, 0); + setvbuf(logfile->fd, NULL, _IOLBF, 0); #endif log_append = 1; } + atomic_rcu_set(&qemu_logfile, logfile); } - qemu_mutex_unlock(&qemu_logfile_mutex); + logfile = qemu_logfile; + if (qemu_logfile && (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) { - qemu_log_close(); + atomic_rcu_set(&qemu_logfile, NULL); + qemu_mutex_unlock(&qemu_logfile_mutex); + call_rcu(logfile, qemu_logfile_free, rcu); + } else { + qemu_mutex_unlock(&qemu_logfile_mutex); } } @@ -247,17 +262,40 @@ out: /* fflush() the log file */ void qemu_log_flush(void) { - fflush(qemu_logfile); + QemuLogFile *logfile; + + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + fflush(logfile->fd); + } + rcu_read_unlock(); +} + +void qemu_logfile_free(QemuLogFile *logfile) +{ + if (logfile) { + if (logfile->fd != stderr) { + fclose(logfile->fd); + } + g_free(logfile); + } } /* Close the log file */ void qemu_log_close(void) { - if (qemu_logfile) { - if (qemu_logfile != stderr) { - fclose(qemu_logfile); - } - qemu_logfile = NULL; + QemuLogFile *logfile; + + qemu_mutex_lock(&qemu_logfile_mutex); + logfile = qemu_logfile; + + if (logfile) { + atomic_rcu_set(&qemu_logfile, NULL); + qemu_mutex_unlock(&qemu_logfile_mutex); + call_rcu(logfile, qemu_logfile_free, rcu); + } else { + qemu_mutex_unlock(&qemu_logfile_mutex); } } diff --git a/include/exec/log.h b/include/exec/log.h index e2cfd436e6..9bd1e4aa20 100644 --- a/include/exec/log.h +++ b/include/exec/log.h @@ -15,8 +15,15 @@ */ static inline void log_cpu_state(CPUState *cpu, int flags) { + QemuLogFile *logfile; + if (qemu_log_enabled()) { - cpu_dump_state(cpu, qemu_logfile, flags); + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + cpu_dump_state(cpu, logfile->fd, flags); + } + rcu_read_unlock(); } } @@ -40,19 +47,37 @@ static inline void log_cpu_state_mask(int mask, CPUState *cpu, int flags) static inline void log_target_disas(CPUState *cpu, target_ulong start, target_ulong len) { - target_disas(qemu_logfile, cpu, start, len); + QemuLogFile *logfile; + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + target_disas(logfile->fd, cpu, start, len); + } + rcu_read_unlock(); } static inline void log_disas(void *code, unsigned long size) { - disas(qemu_logfile, code, size); + QemuLogFile *logfile; + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + disas(logfile->fd, code, size); + } + rcu_read_unlock(); } #if defined(CONFIG_USER_ONLY) /* page_dump() output to the log file: */ static inline void log_page_dump(void) { - page_dump(qemu_logfile); + QemuLogFile *logfile; + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + page_dump(logfile->fd); + } + rcu_read_unlock(); } #endif #endif diff --git a/tcg/tcg.c b/tcg/tcg.c index 5475d49ed1..220eaac7c7 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -2114,9 +2114,17 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs) } if (have_prefs || op->life) { - for (; col < 40; ++col) { - putc(' ', qemu_logfile); + + QemuLogFile *logfile; + + rcu_read_lock(); + logfile = atomic_rcu_read(&qemu_logfile); + if (logfile) { + for (; col < 40; ++col) { + putc(' ', logfile->fd); + } } + rcu_read_unlock(); } if (op->life) {
This now allows changing the logfile while logging is active, and also solves the issue of a seg fault while changing the logfile. Any read access to the qemu_logfile handle will use the rcu_read_lock()/unlock() around the use of the handle. To fetch the handle we will use atomic_rcu_read(). We also in many cases do a check for validity of the logfile handle before using it to deal with the case where the file is closed and set to NULL. The cases where we write to the qemu_logfile will use atomic_rcu_set(). Writers will also use call_rcu() with a newly added qemu_logfile_free function for freeing/closing when readers have finished. Signed-off-by: Robert Foley <robert.foley@linaro.org> --- include/qemu/log.h | 47 ++++++++++++++++++++++++---- util/log.c | 78 ++++++++++++++++++++++++++++++++++------------ include/exec/log.h | 33 +++++++++++++++++--- tcg/tcg.c | 12 +++++-- 4 files changed, 138 insertions(+), 32 deletions(-)