| Message ID | 20191118211528.3221-1-robert.foley@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make the qemu_logfile handle thread safe. | expand |
Robert Foley <robert.foley@linaro.org> writes: > This patch adds thread safety to the qemu_logfile handle. This now > allows changing the logfile while logging is active, and also solves > the issue of a seg fault while changing the logfile. > > This patch adds use of RCU for handling the swap out of the > old qemu_logfile file descriptor. > > Also added a few tests for logfile including changing the logfile > and closing the logfile. > > One change also added for a pre-existing double free issue in > qemu_set_log_filename() uncovered with the new test. > > We also cleaned up the flow of code in qemu_set_log(). This all looks good to me. As we are in hardfreeze now I think it's best we wait for the tree to open before submitting the PR. If no one else wants to pick this up I'll put together a PR. > --- > v3 > - This version of the patch incorporates a few minor changes. > - Change patch which adds qemu_logfile_mutex to remove the > asserts that mutex is initialized, instead we will rely > on the constructor. > - Also added details to commit for patch adding mutex to explain some > unavoidable temporary ugliness that we cleanup in a later patch. > - Change qemu_log_lock() to unconditionally hold the rcu_read_lock() > until qemu_log_unlock(). > - Also changed one use case in target/tilegx/translate.c > to eliminate use of qemu_log_lock()/unlock(). > --- > v2 > - This version of the patch adds some cleanup of code in > qemu_set_log(). > - Also changed the order of patches to move our fix for the > double free issue in qemu_set_log_filename() up to the beginning > of the patch. > --- > v1 > - This version of the patch incorporates changes > from the first round of review. > - It also includes a fix for an issue in > qemu_set_log_filename(). This issue was uncovered > by the test added for this patch. > --- > > Robert Foley (6): > Fix double free issue in qemu_set_log_filename(). > Cleaned up flow of code in qemu_set_log(), to simplify and clarify. > Add a mutex to guarantee single writer to qemu_logfile handle. > qemu_log_lock/unlock now preserves the qemu_logfile handle. > Add use of RCU for qemu_logfile. > Added tests for close and change of logfile. > > accel/tcg/cpu-exec.c | 4 +- > accel/tcg/translate-all.c | 4 +- > accel/tcg/translator.c | 4 +- > exec.c | 4 +- > hw/net/can/can_sja1000.c | 4 +- > include/exec/log.h | 33 +++++++++-- > include/qemu/log.h | 48 +++++++++++++--- > net/can/can_socketcan.c | 5 +- > target/cris/translate.c | 4 +- > target/i386/translate.c | 5 +- > target/lm32/translate.c | 4 +- > target/microblaze/translate.c | 4 +- > target/nios2/translate.c | 4 +- > target/tilegx/translate.c | 6 -- > target/unicore32/translate.c | 4 +- > tcg/tcg.c | 28 ++++++---- > tests/test-logging.c | 80 +++++++++++++++++++++++++++ > util/log.c | 100 ++++++++++++++++++++++++++-------- > 18 files changed, 268 insertions(+), 77 deletions(-) -- Alex Bennée
This patch adds thread safety to the qemu_logfile handle. This now allows changing the logfile while logging is active, and also solves the issue of a seg fault while changing the logfile. This patch adds use of RCU for handling the swap out of the old qemu_logfile file descriptor. Also added a few tests for logfile including changing the logfile and closing the logfile. One change also added for a pre-existing double free issue in qemu_set_log_filename() uncovered with the new test. We also cleaned up the flow of code in qemu_set_log(). --- v3 - This version of the patch incorporates a few minor changes. - Change patch which adds qemu_logfile_mutex to remove the asserts that mutex is initialized, instead we will rely on the constructor. - Also added details to commit for patch adding mutex to explain some unavoidable temporary ugliness that we cleanup in a later patch. - Change qemu_log_lock() to unconditionally hold the rcu_read_lock() until qemu_log_unlock(). - Also changed one use case in target/tilegx/translate.c to eliminate use of qemu_log_lock()/unlock(). --- v2 - This version of the patch adds some cleanup of code in qemu_set_log(). - Also changed the order of patches to move our fix for the double free issue in qemu_set_log_filename() up to the beginning of the patch. --- v1 - This version of the patch incorporates changes from the first round of review. - It also includes a fix for an issue in qemu_set_log_filename(). This issue was uncovered by the test added for this patch. --- Robert Foley (6): Fix double free issue in qemu_set_log_filename(). Cleaned up flow of code in qemu_set_log(), to simplify and clarify. Add a mutex to guarantee single writer to qemu_logfile handle. qemu_log_lock/unlock now preserves the qemu_logfile handle. Add use of RCU for qemu_logfile. Added tests for close and change of logfile. accel/tcg/cpu-exec.c | 4 +- accel/tcg/translate-all.c | 4 +- accel/tcg/translator.c | 4 +- exec.c | 4 +- hw/net/can/can_sja1000.c | 4 +- include/exec/log.h | 33 +++++++++-- include/qemu/log.h | 48 +++++++++++++--- net/can/can_socketcan.c | 5 +- target/cris/translate.c | 4 +- target/i386/translate.c | 5 +- target/lm32/translate.c | 4 +- target/microblaze/translate.c | 4 +- target/nios2/translate.c | 4 +- target/tilegx/translate.c | 6 -- target/unicore32/translate.c | 4 +- tcg/tcg.c | 28 ++++++---- tests/test-logging.c | 80 +++++++++++++++++++++++++++ util/log.c | 100 ++++++++++++++++++++++++++-------- 18 files changed, 268 insertions(+), 77 deletions(-)