RE: [PATCH v2] tcg plugins: expose an API version concept
On Mon, 11 Nov 2019 at 06:35, Alex Bennée wrote: > > This is a very simple versioning API which allows the plugin > infrastructure to check the API a plugin was built against. We also > expose a min/cur API version to the plugin via the info block in case > it wants to avoid using old deprecated APIs in the future. > > Signed-off-by: Alex Bennée > Reviewed-by: Philippe Mathieu-Daudé > > --- > v2 > - error out on missing plugin version symbol > - fix missing symbol on hotblocks.so > - more verbose error text, avoid bad grammar > --- Reviewed-by: Robert Foley Thanks, Rob
[PATCH 2/4] Add use of RCU for qemu_logfile.
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 --- 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; /* 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(_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(_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(_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(_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(_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(_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 (!log
[PATCH 3/4] qemu_log_lock/unlock now preserves the qemu_logfile handle.
qemu_log_lock() now returns a handle and qemu_log_unlock() receives a handle to unlock. This allows for changing the handle during logging and ensures the lock() and unlock() are for the same file. Signed-off-by: Robert Foley --- include/qemu/log.h| 14 +++--- 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 ++-- 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 | 7 --- target/unicore32/translate.c | 4 ++-- tcg/tcg.c | 16 15 files changed, 44 insertions(+), 43 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index 975de18e23..3d0f47a479 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -71,25 +71,25 @@ static inline bool qemu_log_separate(void) * qemu_loglevel is never set when qemu_logfile is unset. */ -static inline void qemu_log_lock(void) +static inline FILE *qemu_log_lock(void) { QemuLogFile *logfile; rcu_read_lock(); logfile = atomic_rcu_read(_logfile); if (logfile) { qemu_flockfile(logfile->fd); +return logfile->fd; } rcu_read_unlock(); +return NULL; } -static inline void qemu_log_unlock(void) +static inline void qemu_log_unlock(FILE *fd) { -QemuLogFile *logfile; -logfile = atomic_rcu_read(_logfile); -if (logfile) { -qemu_funlockfile(logfile->fd); +if (fd) { +qemu_funlockfile(fd); +rcu_read_unlock(); } -rcu_read_unlock(); } /* Logging functions: */ diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c01f59c743..62068d10c3 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -156,7 +156,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) #if defined(DEBUG_DISAS) if (qemu_loglevel_mask(CPU_LOG_TB_CPU) && qemu_log_in_addr_range(itb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); int flags = 0; if (qemu_loglevel_mask(CPU_LOG_TB_FPU)) { flags |= CPU_DUMP_FPU; @@ -165,7 +165,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) flags |= CPU_DUMP_CCOP; #endif log_cpu_state(cpu, flags); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif /* DEBUG_DISAS */ diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 9f48da9472..bb325a2bc4 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1804,7 +1804,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM) && qemu_log_in_addr_range(tb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("OUT: [size=%d]\n", gen_code_size); if (tcg_ctx->data_gen_ptr) { size_t code_size = tcg_ctx->data_gen_ptr - tb->tc.ptr; @@ -1829,7 +1829,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, } qemu_log("\n"); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index f977682be7..603d17ff83 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -138,11 +138,11 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) && qemu_log_in_addr_range(db->pc_first)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("\n"); ops->disas_log(db, cpu); qemu_log("\n"); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif } diff --git a/exec.c b/exec.c index ffdb518535..c994a00f10 100644 --- a/exec.c +++ b/exec.c @@ -1223,13 +1223,13 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...) fprintf(stderr, "\n"); cpu_dump_state(cpu, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP); if (qemu_log_separate()) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("qemu: fatal: "); qemu_log_vprintf(fmt, ap2); qemu_log("\n"); log_cpu_state(cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); qemu_log_close(); } va_end(ap2); diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c index 1f81341554..39c78faf9b 100644 --- a/hw/net/can
[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
[PATCH 4/4] Added tests for close and change of logfile.
One test ensures that the logfile handle is still valid even if the logfile is changed during logging. The other test validates that the logfile handle remains valid under the logfile lock even if the logfile is closed. Signed-off-by: Robert Foley --- tests/test-logging.c | 74 1 file changed, 74 insertions(+) diff --git a/tests/test-logging.c b/tests/test-logging.c index a12585f70a..a3190ff92c 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -108,6 +108,76 @@ static void test_parse_path(gconstpointer data) error_free_or_abort(); } +static void test_logfile_write(gconstpointer data) +{ +QemuLogFile *logfile; +gchar const *dir = data; +Error *err = NULL; +gchar *file_path; +gchar *file_path1; +FILE *orig_fd; + +file_path = g_build_filename(dir, "qemu_test_log_write0.log", NULL); +file_path1 = g_build_filename(dir, "qemu_test_log_write1.log", NULL); + +/* + * Test that even if an open file handle is changed, + * our handle remains valid due to RCU. + */ +qemu_set_log_filename(file_path, ); +g_assert(!err); +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +orig_fd = logfile->fd; +g_assert(logfile && logfile->fd); +fprintf(logfile->fd, "%s 1st write to file\n", __func__); +fflush(logfile->fd); + +/* Change the logfile and ensure that the handle is still valid. */ +qemu_set_log_filename(file_path1, ); +g_assert(!err); +g_assert(logfile->fd == orig_fd); +fprintf(logfile->fd, "%s 2nd write to file\n", __func__); +fflush(logfile->fd); +rcu_read_unlock(); + +g_free(file_path); +g_free(file_path1); +} + +static void test_logfile_lock(gconstpointer data) +{ +FILE *logfile; +gchar const *dir = data; +Error *err = NULL; +gchar *file_path; + +file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL); + +/* + * Test the use of the logfile lock, such + * that even if an open file handle is closed, + * our handle remains valid for use due to RCU. + */ +qemu_set_log_filename(file_path, ); +logfile = qemu_log_lock(); +g_assert(logfile); +fprintf(logfile, "%s 1st write to file\n", __func__); +fflush(logfile); + +/* + * Initiate a close file and make sure our handle remains + * valid since we still have the logfile lock. + */ +qemu_log_close(); +fprintf(logfile, "%s 2nd write to file\n", __func__); +fflush(logfile); +qemu_log_unlock(logfile); + +g_assert(!err); +g_free(file_path); +} + /* Remove a directory and all its entries (non-recursive). */ static void rmdir_full(gchar const *root) { @@ -134,6 +204,10 @@ int main(int argc, char **argv) g_test_add_func("/logging/parse_range", test_parse_range); g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path); +g_test_add_data_func("/logging/logfile_write_path", + tmp_path, test_logfile_write); +g_test_add_data_func("/logging/logfile_lock_path", + tmp_path, test_logfile_lock); rc = g_test_run(); -- 2.17.1
[PATCH 0/4] Make the qemu_logfile handle thread safe.
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. Robert Foley (4): Add a mutex to guarantee single writer to qemu_logfile handle. Add use of RCU for qemu_logfile. qemu_log_lock/unlock now preserves the qemu_logfile handle. 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| 51 +++--- 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 | 7 +-- target/unicore32/translate.c | 4 +- tcg/tcg.c | 28 ++ tests/test-logging.c | 74 ++ util/log.c| 99 --- 18 files changed, 273 insertions(+), 69 deletions(-) -- 2.17.1
Re: logfile issue
On Mon, 4 Nov 2019 at 07:13, Alex Bennée wrote: > I wonder if using RCU to swap out the new and old FD would make things a > bit smoother here? You would have to tweak the qemu_log_lock/unlock > functions to preserve the current FD around the lock and call_rcu a > freeing function when a new handle is set. Thanks for your comments on this. We agree that using RCU is a great solution here. We have this patch well underway and plan to post a patch in the next few days. -Rob On Mon, 4 Nov 2019 at 07:13, Alex Bennée wrote: > > > Robert Foley writes: > > > We hit an issue when trying to change the log file from the monitor > > console. The root of the issue here is that the qemu_logfile handle > > is not thread safe. So when we try to close the file, we end up with > > a seg fault. The full analysis is below along with some possible > > solutions. > > Will plan to post a patch soon, but any comments or opinions on our > > proposed solution would be appreciated. Thanks. > > > > The version of QEMU we are using is: master as of about Oct 15, > > 9020e9526cd08c4dc99d54dba48730de2908c970. > > > > This is what we did to reproduce the issue. > > First we enable logging and select the log file. > > (qemu) log in_asm,out_asm,op > > (qemu) logfile asm.out > > > > Then we start this command in the guest. This just keeps the guest > > performing operations that result in logging to be constantly > > generated. > > $ for i in {0..1000}; do ls -l; done > > > > Next we switch to the monitor console and change the file. > > (qemu) logfile asm_new.log > > > > This action causes a seg fault. Please see the stack trace (below). > > > > The code, which changes the log file unconditionally > > (qemu_set_log_filename()), closes the qemu_logfile, sets it to NULL, > > and then opens the new file. > > Since the file handle is still in use, we end up with a seg fault when > > the code which is trying to log ends up using a NULL file handle. > > Yep, good catch. > > > We are considering a few solutions. > > > > A straightforward solution would be to simply prevent the file from > > being changed while logging is enabled. In other words, force the > > user to first disable logging before changing the log file. > > This solution seems to cover the general case. However, if a user > > were to disable logging and change the log file in quick succession, > > we would still be subject to a similar race. A log call could still > > make it through the logging enable check and proceed to use a file > > handle that gets changed to NULL. > > > > Another option is to add a mutex to prevent the qemu_logfile handle > > from being changed while it is in use. This certainly works and has > > the advantage of being fairly straightforward. Also we are thinking > > that since the mutex would only be used when logging is enabled it has > > the advantage of not having an effect on the normal case performance. > > Another option is to implement a simple atomic ref count and prevent > > the file from being changed while there are outstanding references. > > The mutex is the simplest but I wonder if it show up on heavy usage? The > log file is already doing file locking for the areas that want > contiguous log statements. > > I wonder if using RCU to swap out the new and old FD would make things a > bit smoother here? You would have to tweak the qemu_log_lock/unlock > functions to preserve the current FD around the lock and call_rcu a > freeing function when a new handle is set. > > > We are leaning towards the mutex option, and plan to post a patch > > soon, but would appreciate comments or opinions on this solution. > > > > Thanks, > > Rob Foley > > > > stack trace > > == > > Thread 10 "qemu-system-aar" received signal SIGSEGV, Segmentation fault. > > [Switching to Thread 0x113f9d90 (LWP 9493)] __flockfile > > (stream=0x0) at ../sysdeps/pthread/flockfile.c:27 > > 27 ../sysdeps/pthread/flockfile.c: No such file or directory. > > (gdb) bt > > #0 __flockfile (stream=0x0) at ../sysdeps/pthread/flockfile.c:27 > > #1 0xe0fac8b8 in qemu_flockfile (f=) at > > /home/rob/qemu/qemu_unchanged/include/sysemu/os-posix.h:87 > > #2 qemu_log_lock () at /home/rob/qemu/qemu_unchanged/include/qemu/log.h:57 > > #3 translator_loop (ops=0xe17f1348 , > > db=0x113f9088, db@entry=0x113f9098, > > cpu=cpu@entry=0xaaab0a52bc50, > > tb=tb@entry=0x4c92d000 , > > max_insns=max_insns@entry=512) at > &g
Re: [PATCH 2/4] Add use of RCU for qemu_logfile.
On Thu, 7 Nov 2019 at 11:24, Alex Bennée wrote: > > Robert Foley writes: > > 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(_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(_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(_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(_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(). > > > diff --git a/util/log.c b/util/log.c > > @@ -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(_logfile_mutex); > > if (!qemu_
Re: [PATCH 4/4] Added tests for close and change of logfile.
Agree with all the suggestions below. These are great ideas. Will make the changes. Thanks, -Rob Foley On Thu, 7 Nov 2019 at 11:32, Alex Bennée wrote: > > > Robert Foley writes: > > > One test ensures that the logfile handle is still valid even if > > the logfile is changed during logging. > > The other test validates that the logfile handle remains valid under > > the logfile lock even if the logfile is closed. > > > > Signed-off-by: Robert Foley > > --- > > tests/test-logging.c | 74 > > 1 file changed, 74 insertions(+) > > > > diff --git a/tests/test-logging.c b/tests/test-logging.c > > index a12585f70a..a3190ff92c 100644 > > --- a/tests/test-logging.c > > +++ b/tests/test-logging.c > > @@ -108,6 +108,76 @@ static void test_parse_path(gconstpointer data) > > error_free_or_abort(); > > } > > > > +static void test_logfile_write(gconstpointer data) > > +{ > > +QemuLogFile *logfile; > > +gchar const *dir = data; > > +Error *err = NULL; > > +gchar *file_path; > > +gchar *file_path1; > > with g_autofree char *file_path you can avoid the free down bellow. > > > +FILE *orig_fd; > > + > > +file_path = g_build_filename(dir, "qemu_test_log_write0.log", NULL); > > +file_path1 = g_build_filename(dir, "qemu_test_log_write1.log", NULL); > > + > > +/* > > + * Test that even if an open file handle is changed, > > + * our handle remains valid due to RCU. > > + */ > > +qemu_set_log_filename(file_path, ); > > +g_assert(!err); > > +rcu_read_lock(); > > +logfile = atomic_rcu_read(_logfile); > > +orig_fd = logfile->fd; > > +g_assert(logfile && logfile->fd); > > +fprintf(logfile->fd, "%s 1st write to file\n", __func__); > > +fflush(logfile->fd); > > + > > +/* Change the logfile and ensure that the handle is still valid. */ > > +qemu_set_log_filename(file_path1, ); > > +g_assert(!err); > > Maybe better would be: > > logfile2 = atomic_rcu_read(_logfile); > g_assert(logfile->fd == orig_fd); > g_assert(logfile2->fd != logfile->fd); > fprintf(logfile2->fd, "%s 2nd write to file\n", __func__); > fflush(logfile2->fd); > > > > +g_assert(logfile->fd == orig_fd); > > +fprintf(logfile->fd, "%s 2nd write to file\n", __func__); > > +fflush(logfile->fd); > > +rcu_read_unlock(); > > + > > +g_free(file_path); > > +g_free(file_path1); > > +} > > + > > +static void test_logfile_lock(gconstpointer data) > > +{ > > +FILE *logfile; > > +gchar const *dir = data; > > +Error *err = NULL; > > +gchar *file_path; > > g_autofree > > > + > > +file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL); > > + > > +/* > > + * Test the use of the logfile lock, such > > + * that even if an open file handle is closed, > > + * our handle remains valid for use due to RCU. > > + */ > > +qemu_set_log_filename(file_path, ); > > +logfile = qemu_log_lock(); > > +g_assert(logfile); > > +fprintf(logfile, "%s 1st write to file\n", __func__); > > +fflush(logfile); > > + > > +/* > > + * Initiate a close file and make sure our handle remains > > + * valid since we still have the logfile lock. > > + */ > > +qemu_log_close(); > > +fprintf(logfile, "%s 2nd write to file\n", __func__); > > +fflush(logfile); > > +qemu_log_unlock(logfile); > > + > > +g_assert(!err); > > +g_free(file_path); > > +} > > + > > /* Remove a directory and all its entries (non-recursive). */ > > static void rmdir_full(gchar const *root) > > { > > @@ -134,6 +204,10 @@ int main(int argc, char **argv) > > > > g_test_add_func("/logging/parse_range", test_parse_range); > > g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path); > > +g_test_add_data_func("/logging/logfile_write_path", > > + tmp_path, test_logfile_write); > > +g_test_add_data_func("/logging/logfile_lock_path", > > + tmp_path, test_logfile_lock); > > > > rc = g_test_run(); > > > -- > Alex Bennée
Re: [PATCH 4/4] Added tests for close and change of logfile.
Thanks for providing the stack trace. We debugged this and it seems to come about because of an interesting circumstance. We added our new tests after a pre-existing test, parse_path(), which runs into an issue, a dangling pointer, which could lead to a double free. There were no other tests after the test that ran into the issue, so the double free was not exposed until we added our test which called qemu_set_log_filename(). Upon entry to qemu_set_log_filename() it frees logfilename. In the case where we get an error, we return out without setting the logfilename to NULL. And on next call into this function we will end up with a double free. For a fix, we could put this at the beginning of qemu_set_log_filename(). if (logfilename) { g_free(logfilename); logfilename = NULL; } We were curious to understand why we did not see it in our own testing. Although we did run make check before our first post, we did not see this issue. The docker tests seem to use something like MALLOC_CHECK_, which catches memory issues like this. We will be sure to run the docker tests as well in the future. On Thu, 7 Nov 2019 at 12:26, Alex Bennée wrote: > > > Alex Bennée writes: > > > Robert Foley writes: > > > >> One test ensures that the logfile handle is still valid even if > >> the logfile is changed during logging. > >> The other test validates that the logfile handle remains valid under > >> the logfile lock even if the logfile is closed. > > Also this doesn't see to work: > > 17:24:31 [alex@zen:~/l/q/b/all] review/rcu-logfile|… 2 + ./tests/test-logging > /logging/parse_range: OK > /logging/parse_path: OK > /logging/logfile_write_path: free(): double free detected in tcache 2 > fish: “./tests/test-logging” terminated by signal SIGABRT (Abort) > > in gdb > > Starting program: /home/alex/lsrc/qemu.git/builds/all/tests/test-logging > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > [New Thread 0x76f38700 (LWP 28960)] > /logging/parse_range: OK > /logging/parse_path: OK > /logging/logfile_write_path: free(): double free detected in tcache 2 > > Thread 1 "test-logging" received signal SIGABRT, Aborted. > __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. > (gdb) bt > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x77587535 in __GI_abort () at abort.c:79 > #2 0x775de508 in __libc_message (action=action@entry=do_abort, > fmt=fmt@entry=0x776e928d "%s\n") at ../sysdeps/posix/libc_fatal.c:181 > #3 0x775e4c1a in malloc_printerr (str=str@entry=0x776eaf58 > "free(): double free detected in tcache 2") at malloc.c:5341 > #4 0x775e66fd in _int_free (av=0x77720c40 , > p=0x555cac40, have_lock=) at malloc.c:4193 > #5 0x555614a8 in qemu_set_log_filename (filename=0x555cb110 > "/tmp/qemu-test-logging.RO35A0/qemu_test_log_write0.log", > errp=0x7fffdef0) at /home/alex/lsrc/qemu.git/util/log.c:148 > #6 0xd8be in test_logfile_write (data=0x555c7370) at > /home/alex/lsrc/qemu.git/tests/test-logging.c:127 > #7 0x77cdc15a in test_case_run (tc=0x555c9c60) at > ../../../glib/gtestutils.c:2318 > #8 g_test_run_suite_internal (suite=suite@entry=0x555c8a40, > path=path@entry=0x0) at ../../../glib/gtestutils.c:2403 > #9 0x77cdc014 in g_test_run_suite_internal > (suite=suite@entry=0x555c8a20, path=path@entry=0x0) at > ../../../glib/gtestutils.c:2415 > #10 0x77cdc412 in g_test_run_suite (suite=0x555c8a20) at > ../../../glib/gtestutils.c:2490 > #11 0x77cdc431 in g_test_run () at ../../../glib/gtestutils.c:1755 > #12 0xce07 in main (argc=, argv=) > at /home/alex/lsrc/qemu.git/tests/test-logging.c:212 > > > >> > >> Signed-off-by: Robert Foley > >> --- > >> tests/test-logging.c | 74 > >> 1 file changed, 74 insertions(+) > >> > >> diff --git a/tests/test-logging.c b/tests/test-logging.c > >> index a12585f70a..a3190ff92c 100644 > >> --- a/tests/test-logging.c > >> +++ b/tests/test-logging.c > >> @@ -108,6 +108,76 @@ static void test_parse_path(gconstpointer data) > >> error_free_or_abort(); > >> } > >> > >> +static void test_logfile_write(gconstpointer data) > >> +{ > >> +QemuLogFile *logfile; > >> +gchar const *dir = data; > >> +Error *err = NULL; > >> +gchar *file
Re: [PATCH 3/4] qemu_log_lock/unlock now preserves the qemu_logfile handle.
On Thu, 7 Nov 2019 at 11:25, Alex Bennée wrote: > > Ahh there it is! > > We probably want to put the API change through before we add the RCU > support - so swap the patch order around. > Absolutely, we will swap around the order of the patch. -Rob Foley On Thu, 7 Nov 2019 at 11:25, Alex Bennée wrote: > > > Robert Foley writes: > > > qemu_log_lock() now returns a handle and qemu_log_unlock() receives a > > handle to unlock. This allows for changing the handle during logging > > and ensures the lock() and unlock() are for the same file. > > Ahh there it is! > > We probably want to put the API change through before we add the RCU > support - so swap the patch order around. > > > > > Signed-off-by: Robert Foley > > --- > > include/qemu/log.h| 14 +++--- > > 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 ++-- > > 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 | 7 --- > > target/unicore32/translate.c | 4 ++-- > > tcg/tcg.c | 16 > > 15 files changed, 44 insertions(+), 43 deletions(-) > > > > diff --git a/include/qemu/log.h b/include/qemu/log.h > > index 975de18e23..3d0f47a479 100644 > > --- a/include/qemu/log.h > > +++ b/include/qemu/log.h > > @@ -71,25 +71,25 @@ static inline bool qemu_log_separate(void) > > * qemu_loglevel is never set when qemu_logfile is unset. > > */ > > > > -static inline void qemu_log_lock(void) > > +static inline FILE *qemu_log_lock(void) > > { > > QemuLogFile *logfile; > > rcu_read_lock(); > > logfile = atomic_rcu_read(_logfile); > > if (logfile) { > > qemu_flockfile(logfile->fd); > > +return logfile->fd; > > } > > rcu_read_unlock(); > > +return NULL; > > } > > > > -static inline void qemu_log_unlock(void) > > +static inline void qemu_log_unlock(FILE *fd) > > { > > -QemuLogFile *logfile; > > -logfile = atomic_rcu_read(_logfile); > > -if (logfile) { > > -qemu_funlockfile(logfile->fd); > > +if (fd) { > > +qemu_funlockfile(fd); > > +rcu_read_unlock(); > > } > > -rcu_read_unlock(); > > } > > > > /* Logging functions: */ > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > > index c01f59c743..62068d10c3 100644 > > --- a/accel/tcg/cpu-exec.c > > +++ b/accel/tcg/cpu-exec.c > > @@ -156,7 +156,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState > > *cpu, TranslationBlock *itb) > > #if defined(DEBUG_DISAS) > > if (qemu_loglevel_mask(CPU_LOG_TB_CPU) > > && qemu_log_in_addr_range(itb->pc)) { > > -qemu_log_lock(); > > +FILE *logfile = qemu_log_lock(); > > int flags = 0; > > if (qemu_loglevel_mask(CPU_LOG_TB_FPU)) { > > flags |= CPU_DUMP_FPU; > > @@ -165,7 +165,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState > > *cpu, TranslationBlock *itb) > > flags |= CPU_DUMP_CCOP; > > #endif > > log_cpu_state(cpu, flags); > > -qemu_log_unlock(); > > +qemu_log_unlock(logfile); > > } > > #endif /* DEBUG_DISAS */ > > > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > > index 9f48da9472..bb325a2bc4 100644 > > --- a/accel/tcg/translate-all.c > > +++ b/accel/tcg/translate-all.c > > @@ -1804,7 +1804,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, > > #ifdef DEBUG_DISAS > > if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM) && > > qemu_log_in_addr_range(tb->pc)) { > > -qemu_log_lock(); > > +FILE *logfile = qemu_log_lock(); > > qemu_log("OUT: [size=%d]\n", gen_code_size); > > if (tcg_ctx->data_gen_ptr) { > > size_t code_size = tcg_ctx->data_gen_ptr - tb->tc.ptr; > > @@ -1829,7 +1829,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, > > } > > qemu_log("\n"); > > qemu_log_flush(); &g
logfile issue
We hit an issue when trying to change the log file from the monitor console. The root of the issue here is that the qemu_logfile handle is not thread safe. So when we try to close the file, we end up with a seg fault. The full analysis is below along with some possible solutions. Will plan to post a patch soon, but any comments or opinions on our proposed solution would be appreciated. Thanks. The version of QEMU we are using is: master as of about Oct 15, 9020e9526cd08c4dc99d54dba48730de2908c970. This is what we did to reproduce the issue. First we enable logging and select the log file. (qemu) log in_asm,out_asm,op (qemu) logfile asm.out Then we start this command in the guest. This just keeps the guest performing operations that result in logging to be constantly generated. $ for i in {0..1000}; do ls -l; done Next we switch to the monitor console and change the file. (qemu) logfile asm_new.log This action causes a seg fault. Please see the stack trace (below). The code, which changes the log file unconditionally (qemu_set_log_filename()), closes the qemu_logfile, sets it to NULL, and then opens the new file. Since the file handle is still in use, we end up with a seg fault when the code which is trying to log ends up using a NULL file handle. We are considering a few solutions. A straightforward solution would be to simply prevent the file from being changed while logging is enabled. In other words, force the user to first disable logging before changing the log file. This solution seems to cover the general case. However, if a user were to disable logging and change the log file in quick succession, we would still be subject to a similar race. A log call could still make it through the logging enable check and proceed to use a file handle that gets changed to NULL. Another option is to add a mutex to prevent the qemu_logfile handle from being changed while it is in use. This certainly works and has the advantage of being fairly straightforward. Also we are thinking that since the mutex would only be used when logging is enabled it has the advantage of not having an effect on the normal case performance. Another option is to implement a simple atomic ref count and prevent the file from being changed while there are outstanding references. We are leaning towards the mutex option, and plan to post a patch soon, but would appreciate comments or opinions on this solution. Thanks, Rob Foley stack trace == Thread 10 "qemu-system-aar" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x113f9d90 (LWP 9493)] __flockfile (stream=0x0) at ../sysdeps/pthread/flockfile.c:27 27 ../sysdeps/pthread/flockfile.c: No such file or directory. (gdb) bt #0 __flockfile (stream=0x0) at ../sysdeps/pthread/flockfile.c:27 #1 0xe0fac8b8 in qemu_flockfile (f=) at /home/rob/qemu/qemu_unchanged/include/sysemu/os-posix.h:87 #2 qemu_log_lock () at /home/rob/qemu/qemu_unchanged/include/qemu/log.h:57 #3 translator_loop (ops=0xe17f1348 , db=0x113f9088, db@entry=0x113f9098, cpu=cpu@entry=0xaaab0a52bc50, tb=tb@entry=0x4c92d000 , max_insns=max_insns@entry=512) at /home/rob/qemu/qemu_unchanged/accel/tcg/translator.c:121 #4 0xe10c1c18 in gen_intermediate_code (cpu=cpu@entry=0xaaab0a52bc50, tb=tb@entry=0x4c92d000 , max_insns=max_insns@entry=512) at /home/rob/qemu/qemu_unchanged/target/arm/translate.c:11320 #5 0xe0fab248 in tb_gen_code (cpu=0xaaab0a52bc50, cpu@entry=0xabe2a000, pc=187650897458448, cs_base=65536, flags=43690, cflags=-16252928, cflags@entry=524288) at /home/rob/qemu/qemu_unchanged/accel/tcg/translate-all.c:1738 #6 0xe0fa8e74 in tb_find (cf_mask=524288, tb_exit=0, last_tb=0x4c92cc40 , cpu=0xabe2a000) at /home/rob/qemu/qemu_unchanged/accel/tcg/cpu-exec.c:408 #7 cpu_exec (cpu=0xabe2a000, cpu@entry=0xaaab0a52bc50) at /home/rob/qemu/qemu_unchanged/accel/tcg/cpu-exec.c:730 #8 0xe0f6de24 in tcg_cpu_exec (cpu=0xaaab0a52bc50) at /home/rob/qemu/qemu_unchanged/cpus.c:1454 #9 0xe0f70908 in qemu_tcg_cpu_thread_fn (arg=0xaaab0a52bc50) at /home/rob/qemu/qemu_unchanged/cpus.c:1762 #10 0xe145bd38 in qemu_thread_start (args=) at /home/rob/qemu/qemu_unchanged/util/qemu-thread-posix.c:519 #11 0xabe0a088 in start_thread (arg=0xcc20410f) at pthread_create.c:463 #12 0xabd7a4ec in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:78
Re: FW: [PATCH v1 3/5] docs/devel: update tcg-plugins.rst with API versioning details
On Wed, 13 Nov 2019 at 07:00, Alex Bennée wrote: > > While we are at it fix up the quoted code sections with the inline :: > approach. > > Signed-off-by: Alex Bennée > > --- > v2 > - fix grammar > - mention we also will fail to load outside the range > - clean-up code sections > --- > docs/devel/tcg-plugins.rst | 27 +-- > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst > index b18fb6729e3..718eef00f22 100644 > --- a/docs/devel/tcg-plugins.rst > +++ b/docs/devel/tcg-plugins.rst > @@ -25,6 +25,23 @@ process. However the project reserves the right to change > or break the > API should it need to do so. The best way to avoid this is to submit > your plugin upstream so they can be updated if/when the API changes. > > +API versioning > +-- > + > +All plugins need to declare a symbol which exports the plugin API > +version they were built against. This can be done simply by:: > + > + QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; > + > +The core code will refuse to load a plugin that doesn't export a > +`qemu_plugin_version` symbol or if plugin version is outside of QEMU's > +supported range of API versions. > + > +Additionally the `qemu_info_t` structure which is passed to the > +`qemu_plugin_install` method of a plugin will detail the minimum and > +current API versions supported by QEMU. The API version will be > +incremented if new APIs are added. The minimum API version will be > +incremented if existing APIs are changed or removed. > > Exposure of QEMU internals > -- > @@ -40,16 +57,14 @@ instructions and events are opaque to the plugins > themselves. > Usage > = > > -The QEMU binary needs to be compiled for plugin support: > +The QEMU binary needs to be compiled for plugin support:: > > -:: > -configure --enable-plugins > + configure --enable-plugins > > Once built a program can be run with multiple plugins loaded each with > -their own arguments: > +their own arguments:: > > -:: > -$QEMU $OTHER_QEMU_ARGS \ > + $QEMU $OTHER_QEMU_ARGS \ >-plugin tests/plugin/libhowvec.so,arg=inline,arg=hint \ >-plugin tests/plugin/libhotblocks.so > > -- > 2.20.1 > > Reviewed-by: Robert Foley
Re: [PATCH v1 1/5] Add a mutex to guarantee single writer to qemu_logfile handle.
> > + > > +g_assert(qemu_logfile_mutex.initialized); > > Why the asserts? > > If you want a runtime test, then use the test to initialize it. > Otherwise, trust the constructor. > I see your point here. We can/should just trust the constructor. Will remove the mutex.initialized asserts. Thanks, -Rob On Sat, 16 Nov 2019 at 06:58, Richard Henderson wrote: > > On 11/12/19 4:01 PM, Robert Foley wrote: > > Also added qemu_logfile_init() for initializing the logfile mutex. > > > > Signed-off-by: Robert Foley > > --- > > v1 > > - changed qemu_logfile_init() to use __constructor__. > > --- > > util/log.c | 14 ++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/util/log.c b/util/log.c > > index 1ca13059ee..c25643dc99 100644 > > --- a/util/log.c > > +++ b/util/log.c > > @@ -24,8 +24,10 @@ > > #include "qapi/error.h" > > #include "qemu/cutils.h" > > #include "trace/control.h" > > +#include "qemu/thread.h" > > > > static char *logfilename; > > +static QemuMutex qemu_logfile_mutex; > > FILE *qemu_logfile; > > int qemu_loglevel; > > static int log_append = 0; > > @@ -49,6 +51,11 @@ int qemu_log(const char *fmt, ...) > > return ret; > > } > > > > +static void __attribute__((__constructor__)) qemu_logfile_init(void) > > +{ > > +qemu_mutex_init(_logfile_mutex); > > +} > > + > > static bool log_uses_own_buffers; > > > > /* enable or disable low levels log */ > > @@ -58,6 +65,9 @@ void qemu_set_log(int log_flags) > > #ifdef CONFIG_TRACE_LOG > > qemu_loglevel |= LOG_TRACE; > > #endif > > + > > +g_assert(qemu_logfile_mutex.initialized); > > Why the asserts? > > If you want a runtime test, then use the test to initialize it. > Otherwise, trust the constructor. > > > r~
Re: [PATCH v2 3/6] Add a mutex to guarantee single writer to qemu_logfile handle.
On Mon, 18 Nov 2019 at 07:16, Alex Bennée wrote: > > > +qemu_mutex_lock(_logfile_mutex); > > if (qemu_logfile && !need_to_open_file) { > > +qemu_mutex_unlock(_logfile_mutex); > > qemu_log_close(); > > } else if (!qemu_logfile && need_to_open_file) { > > if (logfilename) { > > @@ -105,6 +115,7 @@ void qemu_set_log(int log_flags) > > #endif > > log_append = 1; > > } > > +qemu_mutex_unlock(_logfile_mutex); > > } > > } > > This looks a bit odd. I can see it's fixed up in a later patch but I > guess the reason is to avoid a double lock when we get to > qemu_log_close(). In the cases of unavoidable temporary ugliness in a > patch series it is best to note the problem and mention it will be > cleaned up by a later patch in the series. > > With an extra comment in the commit message: > > Reviewed-by: Alex Bennée > This is true that given the order of the patches, we needed to add this bit of temporary ugliness to avoid the double lock with this commit, knowing that we would undo these changes later. I will add more details to the commit message. Thanks, -Rob On Mon, 18 Nov 2019 at 07:16, Alex Bennée wrote: > > > Robert Foley writes: > > > Also added qemu_logfile_init() for initializing the logfile mutex. > > > > Signed-off-by: Robert Foley > > --- > > v2 > > - In qemu_set_log() moved location of mutex lock/unlock > > due to cleanup changes. > > --- > > v1 > > - changed qemu_logfile_init() to use __constructor__. > > --- > > util/log.c | 14 ++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/util/log.c b/util/log.c > > index 417d16ec66..91ebb5c924 100644 > > --- a/util/log.c > > +++ b/util/log.c > > @@ -24,8 +24,10 @@ > > #include "qapi/error.h" > > #include "qemu/cutils.h" > > #include "trace/control.h" > > +#include "qemu/thread.h" > > > > static char *logfilename; > > +static QemuMutex qemu_logfile_mutex; > > FILE *qemu_logfile; > > int qemu_loglevel; > > static int log_append = 0; > > @@ -49,6 +51,11 @@ int qemu_log(const char *fmt, ...) > > return ret; > > } > > > > +static void __attribute__((__constructor__)) qemu_logfile_init(void) > > +{ > > +qemu_mutex_init(_logfile_mutex); > > +} > > + > > static bool log_uses_own_buffers; > > > > /* enable or disable low levels log */ > > @@ -70,7 +77,10 @@ void qemu_set_log(int log_flags) > > if (qemu_loglevel && (!is_daemonized() || logfilename)) { > > need_to_open_file = true; > > } > > +g_assert(qemu_logfile_mutex.initialized); > > +qemu_mutex_lock(_logfile_mutex); > > if (qemu_logfile && !need_to_open_file) { > > +qemu_mutex_unlock(_logfile_mutex); > > qemu_log_close(); > > } else if (!qemu_logfile && need_to_open_file) { > > if (logfilename) { > > @@ -105,6 +115,7 @@ void qemu_set_log(int log_flags) > > #endif > > log_append = 1; > > } > > +qemu_mutex_unlock(_logfile_mutex); > > } > > } > > This looks a bit odd. I can see it's fixed up in a later patch but I > guess the reason is to avoid a double lock when we get to > qemu_log_close(). In the cases of unavoidable temporary ugliness in a > patch series it is best to note the problem and mention it will be > cleaned up by a later patch in the series. > > With an extra comment in the commit message: > > Reviewed-by: Alex Bennée > > > > > @@ -240,12 +251,15 @@ void qemu_log_flush(void) > > /* Close the log file */ > > void qemu_log_close(void) > > { > > +g_assert(qemu_logfile_mutex.initialized); > > +qemu_mutex_lock(_logfile_mutex); > > if (qemu_logfile) { > > if (qemu_logfile != stderr) { > > fclose(qemu_logfile); > > } > > qemu_logfile = NULL; > > } > > +qemu_mutex_unlock(_logfile_mutex); > > } > > > > const QEMULogItem qemu_log_items[] = { > > > -- > Alex Bennée
Re: [PATCH v2 5/6] Add use of RCU for qemu_logfile.
On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > > > > +if (logfile) { > > +qemu_flockfile(logfile->fd); > > +return logfile->fd; > > +} else { > > +rcu_read_unlock(); > > As qemu_log_lock() and unlock should be paired we can drop the unlock > here and do an unconditional unlock bellow even if a null fd is passed. > > Otherwise: > > Reviewed-by: Alex Bennée > Sounds reasonable. It sounds like we should change it to be something like this. static inline FILE *qemu_log_lock(void) { QemuLogFile *logfile; rcu_read_lock(); logfile = atomic_rcu_read(_logfile); if (logfile) { qemu_flockfile(logfile->fd); return logfile->fd; } rcu_read_unlock(); return NULL; } I will make the changes. Thanks, -Rob On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > > > Robert Foley 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 > > --- > > v2 > > - No specific changes, just merging in cleanup changes in > > qemu_set_log(). > > --- > > v1 > > - Changes for review comments. > > - Minor changes to definition of QemuLogFile. > > - Changed qemu_log_separate() to fix unbalanced and > > remove qemu_log_enabled() check. > > - changed qemu_log_lock() to include else. > > - make qemu_logfile_free static. > > - use g_assert(logfile) in qemu_logfile_free. > > - Relocated unlock out of if/else in qemu_log_close(), and > > in qemu_set_log(). > > --- > > include/qemu/log.h | 42 +++ > > util/log.c | 72 -- > > include/exec/log.h | 33 ++--- > > tcg/tcg.c | 12 ++-- > > 4 files changed, 126 insertions(+), 33 deletions(-) > > > > diff --git a/include/qemu/log.h b/include/qemu/log.h > > index a7c5b01571..528e1f9dd7 100644 > > --- a/include/qemu/log.h > > +++ b/include/qemu/log.h > > @@ -3,9 +3,16 @@ > > > > /* A small part of this API is split into its own header */ > > #include "qemu/log-for-trace.h" > > +#include "qemu/rcu.h" > > + > > +typedef struct QemuLogFile { > > +struct rcu_head rcu; > > +FILE *fd; > > +} QemuLogFile; > > > > /* Private global variable, don't use */ > > -extern FILE *qemu_logfile; > > +extern QemuLogFile *qemu_logfile; > > + > > > > /* > > * The new API: > > @@ -25,7 +32,16 @@ static inline bool qemu_log_enabled(void) > > */ > > static inline bool qemu_log_separate(void) > > { > > -return qemu_logfile != NULL && qemu_logfile != stderr; > > +QemuLogFile *logfile; > > +bool res = false; > > + > > +rcu_read_lock(); > > +logfile = atomic_rcu_read(_logfile); > > +if (logfile && logfile->fd != stderr) { > > +res = true; > > +} > > +rcu_read_unlock(); > > +return res; > > } > > > > #define CPU_LOG_TB_OUT_ASM (1 << 0) > > @@ -55,14 +71,23 @@ static inline bool qemu_log_separate(void) > > > > static inline FILE *qemu_log_lock(void) > > { > > -qemu_flockfile(qemu_logfile); > > -return logfile->fd; > > +QemuLogFile *logfile; > > +rcu_read_lock(); > > +logfile = atomic_rcu_read(_logfile); > > +if (logfile) { > > +qemu_flockfile(logfile->fd); > > +return logfile->fd; > > +} else { > > +rcu_read_unlock(); > > As qemu_log_lock() and unlock should be paired we can drop the unlock > here and do an unconditional unlock bellow even if a null fd is passed. > > Otherwise: > > Reviewed-by: Alex Bennée > > > +return NULL; > > +} > > } > > > > static inline void qemu_log_unlock(FI
Re: [PATCH v2 5/6] Add use of RCU for qemu_logfile.
On Mon, 18 Nov 2019 at 08:23, Alex Bennée wrote: > > > Robert Foley writes: > > > On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > >> > >> > >> > +if (logfile) { > >> > +qemu_flockfile(logfile->fd); > >> > +return logfile->fd; > >> > +} else { > >> > +rcu_read_unlock(); > >> > >> As qemu_log_lock() and unlock should be paired we can drop the unlock > >> here and do an unconditional unlock bellow even if a null fd is passed. > >> > >> Otherwise: > >> > >> Reviewed-by: Alex Bennée > >> > > Sounds reasonable. It sounds like we should change it to be something > > like this. > > > > static inline FILE *qemu_log_lock(void) > > { > > QemuLogFile *logfile; > > rcu_read_lock(); > > logfile = atomic_rcu_read(_logfile); > > if (logfile) { > > qemu_flockfile(logfile->fd); > > return logfile->fd; > > } > > rcu_read_unlock(); > > return NULL; > > } > > I will make the changes. > > No I mean as you have to call qemu_log_unlock() you can to the unlock > there. You have to hold the lock over the usage of the resource: > OK, got it. We will need to make one other change to target/tilegx/translate.c since it conditionally unlocks depending on what qemu_log_lock() returns. It seems like the right thing to do since it makes all uses of qemu_log_lock() homogeneous in that they will always unconditionally call qemu_log_unlock(). I will make the changes. Thanks, -Rob On Mon, 18 Nov 2019 at 08:23, Alex Bennée wrote: > > > Robert Foley writes: > > > On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > >> > >> > >> > +if (logfile) { > >> > +qemu_flockfile(logfile->fd); > >> > +return logfile->fd; > >> > +} else { > >> > +rcu_read_unlock(); > >> > >> As qemu_log_lock() and unlock should be paired we can drop the unlock > >> here and do an unconditional unlock bellow even if a null fd is passed. > >> > >> Otherwise: > >> > >> Reviewed-by: Alex Bennée > >> > > Sounds reasonable. It sounds like we should change it to be something > > like this. > > > > static inline FILE *qemu_log_lock(void) > > { > > QemuLogFile *logfile; > > rcu_read_lock(); > > logfile = atomic_rcu_read(_logfile); > > if (logfile) { > > qemu_flockfile(logfile->fd); > > return logfile->fd; > > } > > rcu_read_unlock(); > > return NULL; > > } > > I will make the changes. > > No I mean as you have to call qemu_log_unlock() you can to the unlock > there. You have to hold the lock over the usage of the resource: > > static inline FILE *qemu_log_lock(void) > { > QemuLogFile *logfile; > rcu_read_lock(); > logfile = atomic_rcu_read(_logfile); > if (logfile) { > qemu_flockfile(logfile->fd); > return logfile->fd; > } else { > return NULL; > } > } > > static inline void qemu_log_unlock(FILE *fd) > { > if (fd) { > qemu_funlockfile(fd); > } > rcu_read_unlock(); > } > > > > > > Thanks, > > -Rob > > On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > >> > >> > >> Robert Foley 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 > >> > --- > >> > v2 > >> > - No specific changes, just merging in cleanup changes in > >> > qemu_set_log(). > >> > --- >
Re: [PATCH v2 5/6] Add use of RCU for qemu_logfile.
On Mon, 18 Nov 2019 at 11:41, Alex Bennée wrote: > > > >> > > OK, got it. > > We will need to make one other change to target/tilegx/translate.c > > since it conditionally unlocks depending on what qemu_log_lock() > > returns. > > Hmm I'd be tempted to drop the locking all together and drop the final > log("\n") statement. The reason being the translator can longjmp out of > the loop if it attempts to translate an instruction in an inaccessible > page. > Good point. Makes sense to remove the locking and final log("\n"). Will make this change. Thanks, -Rob On Mon, 18 Nov 2019 at 11:41, Alex Bennée wrote: > > > Robert Foley writes: > > > On Mon, 18 Nov 2019 at 08:23, Alex Bennée wrote: > >> > >> > >> Robert Foley writes: > >> > >> > On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > >> >> > >> >> > >> >> > +if (logfile) { > >> >> > +qemu_flockfile(logfile->fd); > >> >> > +return logfile->fd; > >> >> > +} else { > >> >> > +rcu_read_unlock(); > >> >> > >> >> As qemu_log_lock() and unlock should be paired we can drop the unlock > >> >> here and do an unconditional unlock bellow even if a null fd is passed. > >> >> > >> >> Otherwise: > >> >> > >> >> Reviewed-by: Alex Bennée > >> >> > >> > Sounds reasonable. It sounds like we should change it to be something > >> > like this. > >> > > >> > static inline FILE *qemu_log_lock(void) > >> > { > >> > QemuLogFile *logfile; > >> > rcu_read_lock(); > >> > logfile = atomic_rcu_read(_logfile); > >> > if (logfile) { > >> > qemu_flockfile(logfile->fd); > >> > return logfile->fd; > >> > } > >> > rcu_read_unlock(); > >> > return NULL; > >> > } > >> > I will make the changes. > >> > >> No I mean as you have to call qemu_log_unlock() you can to the unlock > >> there. You have to hold the lock over the usage of the resource: > >> > > OK, got it. > > We will need to make one other change to target/tilegx/translate.c > > since it conditionally unlocks depending on what qemu_log_lock() > > returns. > > Hmm I'd be tempted to drop the locking all together and drop the final > log("\n") statement. The reason being the translator can longjmp out of > the loop if it attempts to translate an instruction in an inaccessible > page. > > > > > It seems like the right thing to do since it makes all uses of > > qemu_log_lock() homogeneous in that they will always unconditionally > > call qemu_log_unlock(). > > > > I will make the changes. > > > > Thanks, > > -Rob > > > > On Mon, 18 Nov 2019 at 08:23, Alex Bennée wrote: > >> > >> > >> Robert Foley writes: > >> > >> > On Mon, 18 Nov 2019 at 07:22, Alex Bennée wrote: > >> >> > >> >> > >> >> > +if (logfile) { > >> >> > +qemu_flockfile(logfile->fd); > >> >> > +return logfile->fd; > >> >> > +} else { > >> >> > +rcu_read_unlock(); > >> >> > >> >> As qemu_log_lock() and unlock should be paired we can drop the unlock > >> >> here and do an unconditional unlock bellow even if a null fd is passed. > >> >> > >> >> Otherwise: > >> >> > >> >> Reviewed-by: Alex Bennée > >> >> > >> > Sounds reasonable. It sounds like we should change it to be something > >> > like this. > >> > > >> > static inline FILE *qemu_log_lock(void) > >> > { > >> > QemuLogFile *logfile; > >> > rcu_read_lock(); > >> > logfile = atomic_rcu_read(_logfile); > >> > if (logfile) { > >> > qemu_flockfile(logfile->fd); > >> > return logfile->fd; > >> > } > >> > rcu_read_unlock(); > >> > return NULL; > >> > } > >> > I will make the changes. > >> > >> No I mean as you have to call qemu_log_unlock() you can to the unlock > >> there. You have to hold the lock over the usage of the r
Re: [PATCH v1 3/5] Add use of RCU for qemu_logfile.
On Tue, 12 Nov 2019 at 12:36, Alex Bennée wrote: > > > > } > > +atomic_rcu_set(_logfile, logfile); > > } > > -qemu_mutex_unlock(_logfile_mutex); > > +logfile = qemu_logfile; > > Isn't this read outside of the protection of both rcu_read_lock() and > the mutex? Could that race? This assignment is under the mutex. This change moved the mutex unlock towards the end of the function, just a few lines below the call_rcu(). > > + > > if (qemu_logfile && > > (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) { > > -qemu_log_close(); > > +atomic_rcu_set(_logfile, NULL); > > +call_rcu(logfile, qemu_logfile_free, rcu); > > I wonder if we can re-arrange the logic here so it's lets confusing? For > example the NULL qemu_loglevel can be detected at the start and we > should be able just to squash the current log and reset and go. I'm not > sure about the damonize/stdout check. > > > } > > +qemu_mutex_unlock(_logfile_mutex); > > } > > Absolutely, the logic that was here originally can be improved. We found it confusing also. We could move things around a bit and change it to something like this to help clarify. bool need_to_open_file = false; /* * In all cases we only log if qemu_loglevel is set. * And: * If not daemonized we will always log either to stderr * or to a file (if there is a logfilename). * If we are daemonized, * we will only log if there is a logfilename. */ if (qemu_loglevel && (!is_daemonized() || logfilename) { need_to_open_file = true; } g_assert(qemu_logfile_mutex.initialized); qemu_mutex_lock(_logfile_mutex); if(qemu_logfile && !need_to_open_file) { /* Close file. */ QemuLogFile *logfile = qemu_logfile; atomic_rcu_set(_logfile, NULL); call_rcu(logfile, qemu_logfile_free, rcu); } else if(!qemu_logfile && need_to_open_file) { logfile = g_new0(QemuLogFile, 1); __snip__ existing patch logic for opening the qemu_logfile will be inserted here. } qemu_mutex_unlock(_logfile_mutex); > > { > > char *pidstr; > > + > > g_free(logfilename); > > nit: stray newline Will remove the newline. Thanks, Rob On Tue, 12 Nov 2019 at 12:36, Alex Bennée wrote: > > > Robert Foley 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 > > --- > > v1 > > - Changes for review comments. > > - Minor changes to definition of QemuLogFile. > > - Changed qemu_log_separate() to fix unbalanced and > > remove qemu_log_enabled() check. > > - changed qemu_log_lock() to include else. > > - make qemu_logfile_free static. > > - use g_assert(logfile) in qemu_logfile_free. > > - Relocated unlock out of if/else in qemu_log_close(), and > > in qemu_set_log(). > > --- > > include/qemu/log.h | 42 ++ > > util/log.c | 73 +- > > include/exec/log.h | 33 ++--- > > tcg/tcg.c | 12 ++-- > > 4 files changed, 128 insertions(+), 32 deletions(-) > > > > diff --git a/include/qemu/log.h b/include/qemu/log.h > > index a7c5b01571..528e1f9dd7 100644 > > --- a/include/qemu/log.h > > +++ b/include/qemu/log.h > > @@ -3,9 +3,16 @@ > > > > /* A small part of this API is split into its own header */ > > #include "qemu/log-for-trace.h" > > +#include "qemu/rcu.h" > > + > > +typedef struct QemuLogFile { > > +struct rcu_head rcu; > > +FILE *fd; > > +} QemuLogFile; > > > > /* Private global variable, don't use */ > > -extern FILE *qemu_logfile; > > +extern QemuLogFile *qemu_logfile; > > + > > > > /* > > * The new API: > > @@ -25,7 +32,16 @@ stat
[PATCH v1 4/5] Added tests for close and change of logfile.
One test ensures that the logfile handle is still valid even if the logfile is changed during logging. The other test validates that the logfile handle remains valid under the logfile lock even if the logfile is closed. Signed-off-by: Robert Foley -- v1 - Changes for first round of code review comments. - Added in use of g_autofree, removed the g_free()s. - Added in use of logfile2 and changed sequence of asserts. --- tests/test-logging.c | 80 1 file changed, 80 insertions(+) diff --git a/tests/test-logging.c b/tests/test-logging.c index a12585f70a..1e646f045d 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -108,6 +108,82 @@ static void test_parse_path(gconstpointer data) error_free_or_abort(); } +static void test_logfile_write(gconstpointer data) +{ +QemuLogFile *logfile; +QemuLogFile *logfile2; +gchar const *dir = data; +Error *err = NULL; +g_autofree gchar *file_path; +g_autofree gchar *file_path1; +FILE *orig_fd; + +/* + * Before starting test, set log flags, to ensure the file gets + * opened below with the call to qemu_set_log_filename(). + * In cases where a logging backend other than log is used, + * this is needed. + */ +qemu_set_log(CPU_LOG_TB_OUT_ASM); +file_path = g_build_filename(dir, "qemu_test_log_write0.log", NULL); +file_path1 = g_build_filename(dir, "qemu_test_log_write1.log", NULL); + +/* + * Test that even if an open file handle is changed, + * our handle remains valid due to RCU. + */ +qemu_set_log_filename(file_path, ); +g_assert(!err); +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +orig_fd = logfile->fd; +g_assert(logfile && logfile->fd); +fprintf(logfile->fd, "%s 1st write to file\n", __func__); +fflush(logfile->fd); + +/* Change the logfile and ensure that the handle is still valid. */ +qemu_set_log_filename(file_path1, ); +g_assert(!err); +logfile2 = atomic_rcu_read(_logfile); +g_assert(logfile->fd == orig_fd); +g_assert(logfile2->fd != logfile->fd); +fprintf(logfile->fd, "%s 2nd write to file\n", __func__); +fflush(logfile->fd); +rcu_read_unlock(); +} + +static void test_logfile_lock(gconstpointer data) +{ +FILE *logfile; +gchar const *dir = data; +Error *err = NULL; +g_autofree gchar *file_path; + +file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL); + +/* + * Test the use of the logfile lock, such + * that even if an open file handle is closed, + * our handle remains valid for use due to RCU. + */ +qemu_set_log_filename(file_path, ); +logfile = qemu_log_lock(); +g_assert(logfile); +fprintf(logfile, "%s 1st write to file\n", __func__); +fflush(logfile); + +/* + * Initiate a close file and make sure our handle remains + * valid since we still have the logfile lock. + */ +qemu_log_close(); +fprintf(logfile, "%s 2nd write to file\n", __func__); +fflush(logfile); +qemu_log_unlock(logfile); + +g_assert(!err); +} + /* Remove a directory and all its entries (non-recursive). */ static void rmdir_full(gchar const *root) { @@ -134,6 +210,10 @@ int main(int argc, char **argv) g_test_add_func("/logging/parse_range", test_parse_range); g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path); +g_test_add_data_func("/logging/logfile_write_path", + tmp_path, test_logfile_write); +g_test_add_data_func("/logging/logfile_lock_path", + tmp_path, test_logfile_lock); rc = g_test_run(); -- 2.17.1
[PATCH v1 3/5] Add use of RCU for qemu_logfile.
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 --- v1 - Changes for review comments. - Minor changes to definition of QemuLogFile. - Changed qemu_log_separate() to fix unbalanced and remove qemu_log_enabled() check. - changed qemu_log_lock() to include else. - make qemu_logfile_free static. - use g_assert(logfile) in qemu_logfile_free. - Relocated unlock out of if/else in qemu_log_close(), and in qemu_set_log(). --- include/qemu/log.h | 42 ++ util/log.c | 73 +- include/exec/log.h | 33 ++--- tcg/tcg.c | 12 ++-- 4 files changed, 128 insertions(+), 32 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index a7c5b01571..528e1f9dd7 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -3,9 +3,16 @@ /* A small part of this API is split into its own header */ #include "qemu/log-for-trace.h" +#include "qemu/rcu.h" + +typedef struct QemuLogFile { +struct rcu_head rcu; +FILE *fd; +} QemuLogFile; /* Private global variable, don't use */ -extern FILE *qemu_logfile; +extern QemuLogFile *qemu_logfile; + /* * The new API: @@ -25,7 +32,16 @@ static inline bool qemu_log_enabled(void) */ static inline bool qemu_log_separate(void) { -return qemu_logfile != NULL && qemu_logfile != stderr; +QemuLogFile *logfile; +bool res = false; + +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +if (logfile && logfile->fd != stderr) { +res = true; +} +rcu_read_unlock(); +return res; } #define CPU_LOG_TB_OUT_ASM (1 << 0) @@ -55,14 +71,23 @@ static inline bool qemu_log_separate(void) static inline FILE *qemu_log_lock(void) { -qemu_flockfile(qemu_logfile); -return logfile->fd; +QemuLogFile *logfile; +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +if (logfile) { +qemu_flockfile(logfile->fd); +return logfile->fd; +} else { +rcu_read_unlock(); +return NULL; +} } static inline void qemu_log_unlock(FILE *fd) { if (fd) { qemu_funlockfile(fd); +rcu_read_unlock(); } } @@ -73,9 +98,14 @@ static inline void qemu_log_unlock(FILE *fd) 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(_logfile); +if (logfile) { +vfprintf(logfile->fd, fmt, va); } +rcu_read_unlock(); } /* log only if a bit is set on the current loglevel mask: diff --git a/util/log.c b/util/log.c index c25643dc99..802b8de42e 100644 --- a/util/log.c +++ b/util/log.c @@ -28,7 +28,7 @@ static char *logfilename; static QemuMutex qemu_logfile_mutex; -FILE *qemu_logfile; +QemuLogFile *qemu_logfile; int qemu_loglevel; static int log_append = 0; static GArray *debug_regions; @@ -37,10 +37,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(_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. */ @@ -48,6 +52,7 @@ int qemu_log(const char *fmt, ...) ret = 0; } } +rcu_read_unlock(); return ret; } @@ -56,11 +61,23 @@ static void __attribute__((__constructor__)) qemu_logfile_init(void) qemu_mutex_init(_logfile_mutex); } +static void qemu_logfile_free(QemuLogFile *logfile) +{ +g_assert(logfile); + +if (logfile->fd != stderr) { +fclose(logfile->fd); +} +g_free(logfile); +} + 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; @@ -70,44 +87,50 @@ void qemu_set_log(int log_flags) qemu_mutex_lock(_logfile_mutex); if (!qemu_logfile && (is_daemon
[PATCH v1 0/5] Make the qemu_logfile handle thread safe.
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. --- 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 (5): 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. Fix double free issue in qemu_set_log_filename(). 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| 49 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 | 7 +-- target/unicore32/translate.c | 4 +- tcg/tcg.c | 28 tests/test-logging.c | 80 util/log.c| 86 +++ 18 files changed, 264 insertions(+), 69 deletions(-) -- 2.17.1
[PATCH v1 2/5] qemu_log_lock/unlock now preserves the qemu_logfile handle.
qemu_log_lock() now returns a handle and qemu_log_unlock() receives a handle to unlock. This allows for changing the handle during logging and ensures the lock() and unlock() are for the same file. Signed-off-by: Robert Foley --- v1 - Moved this up in the patch sequence to be before adding RCU for qemu_logfile. --- include/qemu/log.h| 9 ++--- 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 ++-- 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 | 7 --- target/unicore32/translate.c | 4 ++-- tcg/tcg.c | 16 15 files changed, 43 insertions(+), 39 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index a91105b2ad..a7c5b01571 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -53,14 +53,17 @@ static inline bool qemu_log_separate(void) * qemu_loglevel is never set when qemu_logfile is unset. */ -static inline void qemu_log_lock(void) +static inline FILE *qemu_log_lock(void) { qemu_flockfile(qemu_logfile); +return logfile->fd; } -static inline void qemu_log_unlock(void) +static inline void qemu_log_unlock(FILE *fd) { -qemu_funlockfile(qemu_logfile); +if (fd) { +qemu_funlockfile(fd); +} } /* Logging functions: */ diff --git a/net/can/can_socketcan.c b/net/can/can_socketcan.c index 8a6ffad40c..29bfacd4f8 100644 --- a/net/can/can_socketcan.c +++ b/net/can/can_socketcan.c @@ -76,8 +76,7 @@ QEMU_BUILD_BUG_ON(offsetof(qemu_can_frame, data) static void can_host_socketcan_display_msg(struct qemu_can_frame *msg) { int i; - -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("[cansocketcan]: %03X [%01d] %s %s", msg->can_id & QEMU_CAN_EFF_MASK, msg->can_dlc, @@ -89,7 +88,7 @@ static void can_host_socketcan_display_msg(struct qemu_can_frame *msg) } qemu_log("\n"); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); } static void can_host_socketcan_read(void *opaque) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c01f59c743..62068d10c3 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -156,7 +156,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) #if defined(DEBUG_DISAS) if (qemu_loglevel_mask(CPU_LOG_TB_CPU) && qemu_log_in_addr_range(itb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); int flags = 0; if (qemu_loglevel_mask(CPU_LOG_TB_FPU)) { flags |= CPU_DUMP_FPU; @@ -165,7 +165,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) flags |= CPU_DUMP_CCOP; #endif log_cpu_state(cpu, flags); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif /* DEBUG_DISAS */ diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 9f48da9472..bb325a2bc4 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1804,7 +1804,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM) && qemu_log_in_addr_range(tb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("OUT: [size=%d]\n", gen_code_size); if (tcg_ctx->data_gen_ptr) { size_t code_size = tcg_ctx->data_gen_ptr - tb->tc.ptr; @@ -1829,7 +1829,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, } qemu_log("\n"); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index f977682be7..603d17ff83 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -138,11 +138,11 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) && qemu_log_in_addr_range(db->pc_first)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("\n"); ops->disas_log(db, cpu); qemu_log("\n"); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif } diff --git a/exec.c b/exec.c index ffdb518535..c994a00f10 100644 --- a/exec.c +++ b/exec.c @@ -1223,13 +1223,13 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...) fprintf(stderr, "\n"); cpu_dump_state(cpu, stderr,
[PATCH v1 1/5] Add a mutex to guarantee single writer to qemu_logfile handle.
Also added qemu_logfile_init() for initializing the logfile mutex. Signed-off-by: Robert Foley --- v1 - changed qemu_logfile_init() to use __constructor__. --- util/log.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/util/log.c b/util/log.c index 1ca13059ee..c25643dc99 100644 --- a/util/log.c +++ b/util/log.c @@ -24,8 +24,10 @@ #include "qapi/error.h" #include "qemu/cutils.h" #include "trace/control.h" +#include "qemu/thread.h" static char *logfilename; +static QemuMutex qemu_logfile_mutex; FILE *qemu_logfile; int qemu_loglevel; static int log_append = 0; @@ -49,6 +51,11 @@ int qemu_log(const char *fmt, ...) return ret; } +static void __attribute__((__constructor__)) qemu_logfile_init(void) +{ +qemu_mutex_init(_logfile_mutex); +} + static bool log_uses_own_buffers; /* enable or disable low levels log */ @@ -58,6 +65,9 @@ void qemu_set_log(int log_flags) #ifdef CONFIG_TRACE_LOG qemu_loglevel |= LOG_TRACE; #endif + +g_assert(qemu_logfile_mutex.initialized); +qemu_mutex_lock(_logfile_mutex); if (!qemu_logfile && (is_daemonized() ? logfilename != NULL : qemu_loglevel)) { if (logfilename) { @@ -93,6 +103,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(); @@ -230,12 +241,15 @@ void qemu_log_flush(void) /* Close the log file */ void qemu_log_close(void) { +g_assert(qemu_logfile_mutex.initialized); +qemu_mutex_lock(_logfile_mutex); if (qemu_logfile) { if (qemu_logfile != stderr) { fclose(qemu_logfile); } qemu_logfile = NULL; } +qemu_mutex_unlock(_logfile_mutex); } const QEMULogItem qemu_log_items[] = { -- 2.17.1
[PATCH v1 5/5] Fix double free issue in qemu_set_log_filename().
After freeing the logfilename, we set logfilename to NULL, in case of an error which returns without setting logfilename. Signed-off-by: Robert Foley --- v1 - This is new in the patch v1. --- util/log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/log.c b/util/log.c index 802b8de42e..1eed74788c 100644 --- a/util/log.c +++ b/util/log.c @@ -148,6 +148,7 @@ void qemu_set_log_filename(const char *filename, Error **errp) char *pidstr; g_free(logfilename); +logfilename = NULL; pidstr = strstr(filename, "%"); if (pidstr) { -- 2.17.1
[PATCH v2 6/6] Added tests for close and change of logfile.
One test ensures that the logfile handle is still valid even if the logfile is changed during logging. The other test validates that the logfile handle remains valid under the logfile lock even if the logfile is closed. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v1 - Changes for first round of code review comments. - Added in use of g_autofree, removed the g_free()s. - Added in use of logfile2 and changed sequence of asserts. --- tests/test-logging.c | 80 1 file changed, 80 insertions(+) diff --git a/tests/test-logging.c b/tests/test-logging.c index a12585f70a..1e646f045d 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -108,6 +108,82 @@ static void test_parse_path(gconstpointer data) error_free_or_abort(); } +static void test_logfile_write(gconstpointer data) +{ +QemuLogFile *logfile; +QemuLogFile *logfile2; +gchar const *dir = data; +Error *err = NULL; +g_autofree gchar *file_path; +g_autofree gchar *file_path1; +FILE *orig_fd; + +/* + * Before starting test, set log flags, to ensure the file gets + * opened below with the call to qemu_set_log_filename(). + * In cases where a logging backend other than log is used, + * this is needed. + */ +qemu_set_log(CPU_LOG_TB_OUT_ASM); +file_path = g_build_filename(dir, "qemu_test_log_write0.log", NULL); +file_path1 = g_build_filename(dir, "qemu_test_log_write1.log", NULL); + +/* + * Test that even if an open file handle is changed, + * our handle remains valid due to RCU. + */ +qemu_set_log_filename(file_path, ); +g_assert(!err); +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +orig_fd = logfile->fd; +g_assert(logfile && logfile->fd); +fprintf(logfile->fd, "%s 1st write to file\n", __func__); +fflush(logfile->fd); + +/* Change the logfile and ensure that the handle is still valid. */ +qemu_set_log_filename(file_path1, ); +g_assert(!err); +logfile2 = atomic_rcu_read(_logfile); +g_assert(logfile->fd == orig_fd); +g_assert(logfile2->fd != logfile->fd); +fprintf(logfile->fd, "%s 2nd write to file\n", __func__); +fflush(logfile->fd); +rcu_read_unlock(); +} + +static void test_logfile_lock(gconstpointer data) +{ +FILE *logfile; +gchar const *dir = data; +Error *err = NULL; +g_autofree gchar *file_path; + +file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL); + +/* + * Test the use of the logfile lock, such + * that even if an open file handle is closed, + * our handle remains valid for use due to RCU. + */ +qemu_set_log_filename(file_path, ); +logfile = qemu_log_lock(); +g_assert(logfile); +fprintf(logfile, "%s 1st write to file\n", __func__); +fflush(logfile); + +/* + * Initiate a close file and make sure our handle remains + * valid since we still have the logfile lock. + */ +qemu_log_close(); +fprintf(logfile, "%s 2nd write to file\n", __func__); +fflush(logfile); +qemu_log_unlock(logfile); + +g_assert(!err); +} + /* Remove a directory and all its entries (non-recursive). */ static void rmdir_full(gchar const *root) { @@ -134,6 +210,10 @@ int main(int argc, char **argv) g_test_add_func("/logging/parse_range", test_parse_range); g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path); +g_test_add_data_func("/logging/logfile_write_path", + tmp_path, test_logfile_write); +g_test_add_data_func("/logging/logfile_lock_path", + tmp_path, test_logfile_lock); rc = g_test_run(); -- 2.17.1
[PATCH v2 5/6] Add use of RCU for qemu_logfile.
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 --- v2 - No specific changes, just merging in cleanup changes in qemu_set_log(). --- v1 - Changes for review comments. - Minor changes to definition of QemuLogFile. - Changed qemu_log_separate() to fix unbalanced and remove qemu_log_enabled() check. - changed qemu_log_lock() to include else. - make qemu_logfile_free static. - use g_assert(logfile) in qemu_logfile_free. - Relocated unlock out of if/else in qemu_log_close(), and in qemu_set_log(). --- include/qemu/log.h | 42 +++ util/log.c | 72 -- include/exec/log.h | 33 ++--- tcg/tcg.c | 12 ++-- 4 files changed, 126 insertions(+), 33 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index a7c5b01571..528e1f9dd7 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -3,9 +3,16 @@ /* A small part of this API is split into its own header */ #include "qemu/log-for-trace.h" +#include "qemu/rcu.h" + +typedef struct QemuLogFile { +struct rcu_head rcu; +FILE *fd; +} QemuLogFile; /* Private global variable, don't use */ -extern FILE *qemu_logfile; +extern QemuLogFile *qemu_logfile; + /* * The new API: @@ -25,7 +32,16 @@ static inline bool qemu_log_enabled(void) */ static inline bool qemu_log_separate(void) { -return qemu_logfile != NULL && qemu_logfile != stderr; +QemuLogFile *logfile; +bool res = false; + +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +if (logfile && logfile->fd != stderr) { +res = true; +} +rcu_read_unlock(); +return res; } #define CPU_LOG_TB_OUT_ASM (1 << 0) @@ -55,14 +71,23 @@ static inline bool qemu_log_separate(void) static inline FILE *qemu_log_lock(void) { -qemu_flockfile(qemu_logfile); -return logfile->fd; +QemuLogFile *logfile; +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +if (logfile) { +qemu_flockfile(logfile->fd); +return logfile->fd; +} else { +rcu_read_unlock(); +return NULL; +} } static inline void qemu_log_unlock(FILE *fd) { if (fd) { qemu_funlockfile(fd); +rcu_read_unlock(); } } @@ -73,9 +98,14 @@ static inline void qemu_log_unlock(FILE *fd) 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(_logfile); +if (logfile) { +vfprintf(logfile->fd, fmt, va); } +rcu_read_unlock(); } /* log only if a bit is set on the current loglevel mask: diff --git a/util/log.c b/util/log.c index 91ebb5c924..9f9b6b74b7 100644 --- a/util/log.c +++ b/util/log.c @@ -28,7 +28,7 @@ static char *logfilename; static QemuMutex qemu_logfile_mutex; -FILE *qemu_logfile; +QemuLogFile *qemu_logfile; int qemu_loglevel; static int log_append = 0; static GArray *debug_regions; @@ -37,10 +37,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(_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. */ @@ -48,6 +52,7 @@ int qemu_log(const char *fmt, ...) ret = 0; } } +rcu_read_unlock(); return ret; } @@ -56,12 +61,24 @@ static void __attribute__((__constructor__)) qemu_logfile_init(void) qemu_mutex_init(_logfile_mutex); } +static void qemu_logfile_free(QemuLogFile *logfile) +{ +g_assert(logfile); + +if (logfile->fd != stderr) { +fclose(logfile->fd); +} +g_free(logfile); +} + static bool log_uses_own_buffers; /* enable or disable low levels log */ void qemu_set_log(int log_flags) { bool need_to_open_file = false; +QemuLogFile *logfile; + qemu_loglevel = log_flags; #ifdef CONFIG_TRACE_LOG qemu_loglevel |= LOG_TRACE; @@ -80,43 +97,47 @@ void qemu_s
[PATCH v2 3/6] Add a mutex to guarantee single writer to qemu_logfile handle.
Also added qemu_logfile_init() for initializing the logfile mutex. Signed-off-by: Robert Foley --- v2 - In qemu_set_log() moved location of mutex lock/unlock due to cleanup changes. --- v1 - changed qemu_logfile_init() to use __constructor__. --- util/log.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/util/log.c b/util/log.c index 417d16ec66..91ebb5c924 100644 --- a/util/log.c +++ b/util/log.c @@ -24,8 +24,10 @@ #include "qapi/error.h" #include "qemu/cutils.h" #include "trace/control.h" +#include "qemu/thread.h" static char *logfilename; +static QemuMutex qemu_logfile_mutex; FILE *qemu_logfile; int qemu_loglevel; static int log_append = 0; @@ -49,6 +51,11 @@ int qemu_log(const char *fmt, ...) return ret; } +static void __attribute__((__constructor__)) qemu_logfile_init(void) +{ +qemu_mutex_init(_logfile_mutex); +} + static bool log_uses_own_buffers; /* enable or disable low levels log */ @@ -70,7 +77,10 @@ void qemu_set_log(int log_flags) if (qemu_loglevel && (!is_daemonized() || logfilename)) { need_to_open_file = true; } +g_assert(qemu_logfile_mutex.initialized); +qemu_mutex_lock(_logfile_mutex); if (qemu_logfile && !need_to_open_file) { +qemu_mutex_unlock(_logfile_mutex); qemu_log_close(); } else if (!qemu_logfile && need_to_open_file) { if (logfilename) { @@ -105,6 +115,7 @@ void qemu_set_log(int log_flags) #endif log_append = 1; } +qemu_mutex_unlock(_logfile_mutex); } } @@ -240,12 +251,15 @@ void qemu_log_flush(void) /* Close the log file */ void qemu_log_close(void) { +g_assert(qemu_logfile_mutex.initialized); +qemu_mutex_lock(_logfile_mutex); if (qemu_logfile) { if (qemu_logfile != stderr) { fclose(qemu_logfile); } qemu_logfile = NULL; } +qemu_mutex_unlock(_logfile_mutex); } const QEMULogItem qemu_log_items[] = { -- 2.17.1
[PATCH v2 2/6] Cleaned up flow of code in qemu_set_log(), to simplify and clarify.
Also added some explanation of the reasoning behind the branches. Signed-off-by: Robert Foley --- v2 - This is new in patch v2. --- util/log.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/util/log.c b/util/log.c index 4316fe74ee..417d16ec66 100644 --- a/util/log.c +++ b/util/log.c @@ -54,12 +54,25 @@ static bool log_uses_own_buffers; /* enable or disable low levels log */ void qemu_set_log(int log_flags) { +bool need_to_open_file = false; qemu_loglevel = log_flags; #ifdef CONFIG_TRACE_LOG qemu_loglevel |= LOG_TRACE; #endif -if (!qemu_logfile && -(is_daemonized() ? logfilename != NULL : qemu_loglevel)) { +/* + * In all cases we only log if qemu_loglevel is set. + * Also: + * If not daemonized we will always log either to stderr + * or to a file (if there is a logfilename). + * If we are daemonized, + * we will only log if there is a logfilename. + */ +if (qemu_loglevel && (!is_daemonized() || logfilename)) { +need_to_open_file = true; +} +if (qemu_logfile && !need_to_open_file) { +qemu_log_close(); +} else if (!qemu_logfile && need_to_open_file) { if (logfilename) { qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); if (!qemu_logfile) { @@ -93,10 +106,6 @@ void qemu_set_log(int log_flags) log_append = 1; } } -if (qemu_logfile && -(is_daemonized() ? logfilename == NULL : !qemu_loglevel)) { -qemu_log_close(); -} } void qemu_log_needs_buffers(void) -- 2.17.1
[PATCH v2 4/6] qemu_log_lock/unlock now preserves the qemu_logfile handle.
qemu_log_lock() now returns a handle and qemu_log_unlock() receives a handle to unlock. This allows for changing the handle during logging and ensures the lock() and unlock() are for the same file. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v1 - Moved this up in the patch sequence to be before adding RCU for qemu_logfile. --- include/qemu/log.h| 9 ++--- 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 ++-- 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 | 7 --- target/unicore32/translate.c | 4 ++-- tcg/tcg.c | 16 15 files changed, 43 insertions(+), 39 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index a91105b2ad..a7c5b01571 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -53,14 +53,17 @@ static inline bool qemu_log_separate(void) * qemu_loglevel is never set when qemu_logfile is unset. */ -static inline void qemu_log_lock(void) +static inline FILE *qemu_log_lock(void) { qemu_flockfile(qemu_logfile); +return logfile->fd; } -static inline void qemu_log_unlock(void) +static inline void qemu_log_unlock(FILE *fd) { -qemu_funlockfile(qemu_logfile); +if (fd) { +qemu_funlockfile(fd); +} } /* Logging functions: */ diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c01f59c743..62068d10c3 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -156,7 +156,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) #if defined(DEBUG_DISAS) if (qemu_loglevel_mask(CPU_LOG_TB_CPU) && qemu_log_in_addr_range(itb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); int flags = 0; if (qemu_loglevel_mask(CPU_LOG_TB_FPU)) { flags |= CPU_DUMP_FPU; @@ -165,7 +165,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) flags |= CPU_DUMP_CCOP; #endif log_cpu_state(cpu, flags); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif /* DEBUG_DISAS */ diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 9f48da9472..bb325a2bc4 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1804,7 +1804,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM) && qemu_log_in_addr_range(tb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("OUT: [size=%d]\n", gen_code_size); if (tcg_ctx->data_gen_ptr) { size_t code_size = tcg_ctx->data_gen_ptr - tb->tc.ptr; @@ -1829,7 +1829,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, } qemu_log("\n"); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index f977682be7..603d17ff83 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -138,11 +138,11 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) && qemu_log_in_addr_range(db->pc_first)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("\n"); ops->disas_log(db, cpu); qemu_log("\n"); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif } diff --git a/exec.c b/exec.c index ffdb518535..c994a00f10 100644 --- a/exec.c +++ b/exec.c @@ -1223,13 +1223,13 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...) fprintf(stderr, "\n"); cpu_dump_state(cpu, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP); if (qemu_log_separate()) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("qemu: fatal: "); qemu_log_vprintf(fmt, ap2); qemu_log("\n"); log_cpu_state(cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); qemu_log_close(); } va_end(ap2); diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c index 1f81341554..39c78faf9b 100644 --- a/hw/net/can/can_sja1000.c +++ b/hw/net/can/can_sja1000.c @@ -247,8 +247,8 @@ int can_sja_accept_filter(CanSJA1000State *s, static void can_display_msg(const char *prefix, const qemu_can_frame *msg) { int i; +
[PATCH v2 0/6] Make the qemu_logfile handle thread safe.
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(). --- 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| 49 +--- 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 | 7 ++- target/unicore32/translate.c | 4 +- tcg/tcg.c | 28 ++ tests/test-logging.c | 80 ++ util/log.c| 102 ++ 18 files changed, 275 insertions(+), 74 deletions(-) -- 2.17.1
[PATCH v2 1/6] Fix double free issue in qemu_set_log_filename().
After freeing the logfilename, we set logfilename to NULL, in case of an error which returns without setting logfilename. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v2 - moved this change to the beginning of the patch series. --- v1 - This is new in the patch v1. --- util/log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/log.c b/util/log.c index 1ca13059ee..4316fe74ee 100644 --- a/util/log.c +++ b/util/log.c @@ -113,6 +113,7 @@ void qemu_set_log_filename(const char *filename, Error **errp) { char *pidstr; g_free(logfilename); +logfilename = NULL; pidstr = strstr(filename, "%"); if (pidstr) { -- 2.17.1
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 4/4] Added tests for close and change of logfile.
On Thu, 7 Nov 2019 at 15:12, Alex Bennée wrote: > > > > For a fix, we could put this at the beginning of qemu_set_log_filename(). > > if (logfilename) { > > g_free(logfilename); > > logfilename = NULL; > > } > > g_free(logfilename) should be safe against NULL. However we need to > ensure that logfilename is NULL'ed after it so we don't see the double > free. > This makes sense, will remove the if (logfilename). > > We were curious to understand why we did not see it in our own > > testing. Although we did run make check before our first post, we did > > not see this issue. The docker tests seem to use something like > > MALLOC_CHECK_, which catches memory issues like this. We will be > > sure to run the docker tests as well in the future. > > I was just running in my normal checkout - it could depend on how glibc > was built for your system though. Mine is Debian Buster. > Interesting. We had assumed it was just the way we were running the test. But it sounds like something about our particular setup. I'm using Ubuntu bionic on an aarch64 system. Will look further. Thanks, -Rob Foley On Thu, 7 Nov 2019 at 15:12, Alex Bennée wrote: > > > Robert Foley writes: > > > Thanks for providing the stack trace. > > > > We debugged this and it seems to come about because of an interesting > > circumstance. We added our new tests after a pre-existing test, > > parse_path(), which runs into an issue, a dangling pointer, which > > could lead to a double free. There were no other tests after the test > > that ran into the issue, so the double free was not exposed until we > > added our test which called qemu_set_log_filename(). > > > > Upon entry to qemu_set_log_filename() it frees logfilename. In the > > case where we get an error, we return out without setting the > > logfilename to NULL. > > And on next call into this function we will end up with a double free. > > > > For a fix, we could put this at the beginning of qemu_set_log_filename(). > > if (logfilename) { > > g_free(logfilename); > > logfilename = NULL; > > } > > g_free(logfilename) should be safe against NULL. However we need to > ensure that logfilename is NULL'ed after it so we don't see the double > free. > > > We were curious to understand why we did not see it in our own > > testing. Although we did run make check before our first post, we did > > not see this issue. The docker tests seem to use something like > > MALLOC_CHECK_, which catches memory issues like this. We will be > > sure to run the docker tests as well in the future. > > I was just running in my normal checkout - it could depend on how glibc > was built for your system though. Mine is Debian Buster. > > > > > On Thu, 7 Nov 2019 at 12:26, Alex Bennée wrote: > >> > >> > >> Alex Bennée writes: > >> > >> > Robert Foley writes: > >> > > >> >> One test ensures that the logfile handle is still valid even if > >> >> the logfile is changed during logging. > >> >> The other test validates that the logfile handle remains valid under > >> >> the logfile lock even if the logfile is closed. > >> > >> Also this doesn't see to work: > >> > >> 17:24:31 [alex@zen:~/l/q/b/all] review/rcu-logfile|… 2 + > >> ./tests/test-logging > >> /logging/parse_range: OK > >> /logging/parse_path: OK > >> /logging/logfile_write_path: free(): double free detected in tcache 2 > >> fish: “./tests/test-logging” terminated by signal SIGABRT (Abort) > >> > >> in gdb > >> > >> Starting program: /home/alex/lsrc/qemu.git/builds/all/tests/test-logging > >> [Thread debugging using libthread_db enabled] > >> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > >> [New Thread 0x76f38700 (LWP 28960)] > >> /logging/parse_range: OK > >> /logging/parse_path: OK > >> /logging/logfile_write_path: free(): double free detected in tcache 2 > >> > >> Thread 1 "test-logging" received signal SIGABRT, Aborted. > >> __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > >> 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. > >> (gdb) bt > >> #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > >> #1 0x77587535 in __GI_abort () at abort.c:79 > >> #2 0x775de508 in __libc_message (action=action@entry=do_abort, > >> fmt
[PATCH v3 4/6] qemu_log_lock/unlock now preserves the qemu_logfile handle.
qemu_log_lock() now returns a handle and qemu_log_unlock() receives a handle to unlock. This allows for changing the handle during logging and ensures the lock() and unlock() are for the same file. Also in target/tilegx/translate.c removed the qemu_log_lock()/unlock() calls (and the log("\n")), since the translator can longjmp out of the loop if it attempts to translate an instruction in an inaccessible page. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée Reviewed-by: Richard Henderson --- v3 - Changes to one use case to remove use of qemu_log_lock()/unlock(). Only change was target/tilegx/translate.c --- v1 - Moved this up in the patch sequence to be before adding RCU for qemu_logfile. --- include/qemu/log.h| 9 ++--- target/tilegx/translate.c | 6 -- 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 ++-- 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/unicore32/translate.c | 4 ++-- tcg/tcg.c | 16 15 files changed, 39 insertions(+), 42 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index a91105b2ad..a7c5b01571 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -53,14 +53,17 @@ static inline bool qemu_log_separate(void) * qemu_loglevel is never set when qemu_logfile is unset. */ -static inline void qemu_log_lock(void) +static inline FILE *qemu_log_lock(void) { qemu_flockfile(qemu_logfile); +return logfile->fd; } -static inline void qemu_log_unlock(void) +static inline void qemu_log_unlock(FILE *fd) { -qemu_funlockfile(qemu_logfile); +if (fd) { +qemu_funlockfile(fd); +} } /* Logging functions: */ diff --git a/target/tilegx/translate.c b/target/tilegx/translate.c index 68dd4aa2d8..abce7e1c75 100644 --- a/target/tilegx/translate.c +++ b/target/tilegx/translate.c @@ -2388,7 +2388,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) dc->zero = NULL; if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) { -qemu_log_lock(); qemu_log("IN: %s\n", lookup_symbol(pc_start)); } gen_tb_start(tb); @@ -2417,11 +2416,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) gen_tb_end(tb, num_insns); tb->size = dc->pc - pc_start; tb->icount = num_insns; - -if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) { -qemu_log("\n"); -qemu_log_unlock(); -} } void restore_state_to_opc(CPUTLGState *env, TranslationBlock *tb, diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c01f59c743..62068d10c3 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -156,7 +156,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) #if defined(DEBUG_DISAS) if (qemu_loglevel_mask(CPU_LOG_TB_CPU) && qemu_log_in_addr_range(itb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); int flags = 0; if (qemu_loglevel_mask(CPU_LOG_TB_FPU)) { flags |= CPU_DUMP_FPU; @@ -165,7 +165,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) flags |= CPU_DUMP_CCOP; #endif log_cpu_state(cpu, flags); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif /* DEBUG_DISAS */ diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 9f48da9472..bb325a2bc4 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1804,7 +1804,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM) && qemu_log_in_addr_range(tb->pc)) { -qemu_log_lock(); +FILE *logfile = qemu_log_lock(); qemu_log("OUT: [size=%d]\n", gen_code_size); if (tcg_ctx->data_gen_ptr) { size_t code_size = tcg_ctx->data_gen_ptr - tb->tc.ptr; @@ -1829,7 +1829,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, } qemu_log("\n"); qemu_log_flush(); -qemu_log_unlock(); +qemu_log_unlock(logfile); } #endif diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index f977682be7..603d17ff83 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -138,11 +138,11 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, #ifdef DEBUG_DISAS if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) && qemu_log_in_addr_range(db->pc_first)) { -qemu_log_lock(); +
[PATCH v3 5/6] Add use of RCU for qemu_logfile.
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 Reviewed-by: Alex Bennée --- v3 - Changes for qemu_log_lock() to unconditionally hold the rcu_read_lock() until qemu_log_unlock() - Changed qemu_log_unlock() to unconditionally rcu_read_unlock() --- v2 - No specific changes, just merging in cleanup changes in qemu_set_log(). --- v1 - Changes for review comments. - Minor changes to definition of QemuLogFile. - Changed qemu_log_separate() to fix unbalanced and remove qemu_log_enabled() check. - changed qemu_log_lock() to include else. - make qemu_logfile_free static. - use g_assert(logfile) in qemu_logfile_free. - Relocated unlock out of if/else in qemu_log_close(), and in qemu_set_log(). --- include/qemu/log.h | 41 ++ util/log.c | 72 -- include/exec/log.h | 33 ++--- tcg/tcg.c | 12 ++-- 4 files changed, 125 insertions(+), 33 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index a7c5b01571..e0f4e40628 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -3,9 +3,16 @@ /* A small part of this API is split into its own header */ #include "qemu/log-for-trace.h" +#include "qemu/rcu.h" + +typedef struct QemuLogFile { +struct rcu_head rcu; +FILE *fd; +} QemuLogFile; /* Private global variable, don't use */ -extern FILE *qemu_logfile; +extern QemuLogFile *qemu_logfile; + /* * The new API: @@ -25,7 +32,16 @@ static inline bool qemu_log_enabled(void) */ static inline bool qemu_log_separate(void) { -return qemu_logfile != NULL && qemu_logfile != stderr; +QemuLogFile *logfile; +bool res = false; + +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +if (logfile && logfile->fd != stderr) { +res = true; +} +rcu_read_unlock(); +return res; } #define CPU_LOG_TB_OUT_ASM (1 << 0) @@ -55,8 +71,15 @@ static inline bool qemu_log_separate(void) static inline FILE *qemu_log_lock(void) { -qemu_flockfile(qemu_logfile); -return logfile->fd; +QemuLogFile *logfile; +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +if (logfile) { +qemu_flockfile(logfile->fd); +return logfile->fd; +} else { +return NULL; +} } static inline void qemu_log_unlock(FILE *fd) @@ -64,6 +87,7 @@ static inline void qemu_log_unlock(FILE *fd) if (fd) { qemu_funlockfile(fd); } +rcu_read_unlock(); } /* Logging functions: */ @@ -73,9 +97,14 @@ static inline void qemu_log_unlock(FILE *fd) 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(_logfile); +if (logfile) { +vfprintf(logfile->fd, fmt, va); } +rcu_read_unlock(); } /* log only if a bit is set on the current loglevel mask: diff --git a/util/log.c b/util/log.c index 953a66b5a8..867264da8d 100644 --- a/util/log.c +++ b/util/log.c @@ -28,7 +28,7 @@ static char *logfilename; static QemuMutex qemu_logfile_mutex; -FILE *qemu_logfile; +QemuLogFile *qemu_logfile; int qemu_loglevel; static int log_append = 0; static GArray *debug_regions; @@ -37,10 +37,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(_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. */ @@ -48,6 +52,7 @@ int qemu_log(const char *fmt, ...) ret = 0; } } +rcu_read_unlock(); return ret; } @@ -56,12 +61,24 @@ static void __attribute__((__constructor__)) qemu_logfile_init(void) qemu_mutex_init(_logfile_mutex); } +static void qemu_logfile_free(QemuLogFile *logfile) +{ +g_assert(logfile); + +if (logfile->fd != stderr) { +fclose(logfile->fd); +} +g_free(logfile); +} + static bool log_uses_own_buffers
[PATCH v3 3/6] Add a mutex to guarantee single writer to qemu_logfile handle.
Also added qemu_logfile_init() for initializing the logfile mutex. Note that inside qemu_set_log() we needed to add a pair of qemu_mutex_unlock() calls in order to avoid a double lock in qemu_log_close(). This unavoidable temporary ugliness will be cleaned up in a later patch in this series. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v3 - Removed assert that mutex is initialized. We will rely on the constructor. - Also added details in the commit message regarding the temporary ugliness that will be cleaned up in a later patch. --- v2 - In qemu_set_log() moved location of mutex lock/unlock due to cleanup changes. --- v1 - changed qemu_logfile_init() to use __constructor__. --- util/log.c | 12 1 file changed, 12 insertions(+) diff --git a/util/log.c b/util/log.c index 417d16ec66..953a66b5a8 100644 --- a/util/log.c +++ b/util/log.c @@ -24,8 +24,10 @@ #include "qapi/error.h" #include "qemu/cutils.h" #include "trace/control.h" +#include "qemu/thread.h" static char *logfilename; +static QemuMutex qemu_logfile_mutex; FILE *qemu_logfile; int qemu_loglevel; static int log_append = 0; @@ -49,6 +51,11 @@ int qemu_log(const char *fmt, ...) return ret; } +static void __attribute__((__constructor__)) qemu_logfile_init(void) +{ +qemu_mutex_init(_logfile_mutex); +} + static bool log_uses_own_buffers; /* enable or disable low levels log */ @@ -70,7 +77,9 @@ void qemu_set_log(int log_flags) if (qemu_loglevel && (!is_daemonized() || logfilename)) { need_to_open_file = true; } +qemu_mutex_lock(_logfile_mutex); if (qemu_logfile && !need_to_open_file) { +qemu_mutex_unlock(_logfile_mutex); qemu_log_close(); } else if (!qemu_logfile && need_to_open_file) { if (logfilename) { @@ -105,6 +114,7 @@ void qemu_set_log(int log_flags) #endif log_append = 1; } +qemu_mutex_unlock(_logfile_mutex); } } @@ -240,12 +250,14 @@ void qemu_log_flush(void) /* Close the log file */ void qemu_log_close(void) { +qemu_mutex_lock(_logfile_mutex); if (qemu_logfile) { if (qemu_logfile != stderr) { fclose(qemu_logfile); } qemu_logfile = NULL; } +qemu_mutex_unlock(_logfile_mutex); } const QEMULogItem qemu_log_items[] = { -- 2.17.1
[PATCH v3 6/6] Added tests for close and change of logfile.
One test ensures that the logfile handle is still valid even if the logfile is changed during logging. The other test validates that the logfile handle remains valid under the logfile lock even if the logfile is closed. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v1 - Changes for first round of code review comments. - Added in use of g_autofree, removed the g_free()s. - Added in use of logfile2 and changed sequence of asserts. --- tests/test-logging.c | 80 1 file changed, 80 insertions(+) diff --git a/tests/test-logging.c b/tests/test-logging.c index a12585f70a..1e646f045d 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -108,6 +108,82 @@ static void test_parse_path(gconstpointer data) error_free_or_abort(); } +static void test_logfile_write(gconstpointer data) +{ +QemuLogFile *logfile; +QemuLogFile *logfile2; +gchar const *dir = data; +Error *err = NULL; +g_autofree gchar *file_path; +g_autofree gchar *file_path1; +FILE *orig_fd; + +/* + * Before starting test, set log flags, to ensure the file gets + * opened below with the call to qemu_set_log_filename(). + * In cases where a logging backend other than log is used, + * this is needed. + */ +qemu_set_log(CPU_LOG_TB_OUT_ASM); +file_path = g_build_filename(dir, "qemu_test_log_write0.log", NULL); +file_path1 = g_build_filename(dir, "qemu_test_log_write1.log", NULL); + +/* + * Test that even if an open file handle is changed, + * our handle remains valid due to RCU. + */ +qemu_set_log_filename(file_path, ); +g_assert(!err); +rcu_read_lock(); +logfile = atomic_rcu_read(_logfile); +orig_fd = logfile->fd; +g_assert(logfile && logfile->fd); +fprintf(logfile->fd, "%s 1st write to file\n", __func__); +fflush(logfile->fd); + +/* Change the logfile and ensure that the handle is still valid. */ +qemu_set_log_filename(file_path1, ); +g_assert(!err); +logfile2 = atomic_rcu_read(_logfile); +g_assert(logfile->fd == orig_fd); +g_assert(logfile2->fd != logfile->fd); +fprintf(logfile->fd, "%s 2nd write to file\n", __func__); +fflush(logfile->fd); +rcu_read_unlock(); +} + +static void test_logfile_lock(gconstpointer data) +{ +FILE *logfile; +gchar const *dir = data; +Error *err = NULL; +g_autofree gchar *file_path; + +file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL); + +/* + * Test the use of the logfile lock, such + * that even if an open file handle is closed, + * our handle remains valid for use due to RCU. + */ +qemu_set_log_filename(file_path, ); +logfile = qemu_log_lock(); +g_assert(logfile); +fprintf(logfile, "%s 1st write to file\n", __func__); +fflush(logfile); + +/* + * Initiate a close file and make sure our handle remains + * valid since we still have the logfile lock. + */ +qemu_log_close(); +fprintf(logfile, "%s 2nd write to file\n", __func__); +fflush(logfile); +qemu_log_unlock(logfile); + +g_assert(!err); +} + /* Remove a directory and all its entries (non-recursive). */ static void rmdir_full(gchar const *root) { @@ -134,6 +210,10 @@ int main(int argc, char **argv) g_test_add_func("/logging/parse_range", test_parse_range); g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path); +g_test_add_data_func("/logging/logfile_write_path", + tmp_path, test_logfile_write); +g_test_add_data_func("/logging/logfile_lock_path", + tmp_path, test_logfile_lock); rc = g_test_run(); -- 2.17.1
[PATCH v3 1/6] Fix double free issue in qemu_set_log_filename().
After freeing the logfilename, we set logfilename to NULL, in case of an error which returns without setting logfilename. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v2 - moved this change to the beginning of the patch series. --- v1 - This is new in the patch v1. --- util/log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/log.c b/util/log.c index 1ca13059ee..4316fe74ee 100644 --- a/util/log.c +++ b/util/log.c @@ -113,6 +113,7 @@ void qemu_set_log_filename(const char *filename, Error **errp) { char *pidstr; g_free(logfilename); +logfilename = NULL; pidstr = strstr(filename, "%"); if (pidstr) { -- 2.17.1
[PATCH v3 2/6] Cleaned up flow of code in qemu_set_log(), to simplify and clarify.
Also added some explanation of the reasoning behind the branches. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée --- v2 - This is new in patch v2. --- util/log.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/util/log.c b/util/log.c index 4316fe74ee..417d16ec66 100644 --- a/util/log.c +++ b/util/log.c @@ -54,12 +54,25 @@ static bool log_uses_own_buffers; /* enable or disable low levels log */ void qemu_set_log(int log_flags) { +bool need_to_open_file = false; qemu_loglevel = log_flags; #ifdef CONFIG_TRACE_LOG qemu_loglevel |= LOG_TRACE; #endif -if (!qemu_logfile && -(is_daemonized() ? logfilename != NULL : qemu_loglevel)) { +/* + * In all cases we only log if qemu_loglevel is set. + * Also: + * If not daemonized we will always log either to stderr + * or to a file (if there is a logfilename). + * If we are daemonized, + * we will only log if there is a logfilename. + */ +if (qemu_loglevel && (!is_daemonized() || logfilename)) { +need_to_open_file = true; +} +if (qemu_logfile && !need_to_open_file) { +qemu_log_close(); +} else if (!qemu_logfile && need_to_open_file) { if (logfilename) { qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); if (!qemu_logfile) { @@ -93,10 +106,6 @@ void qemu_set_log(int log_flags) log_append = 1; } } -if (qemu_logfile && -(is_daemonized() ? logfilename == NULL : !qemu_loglevel)) { -qemu_log_close(); -} } void qemu_log_needs_buffers(void) -- 2.17.1
[PATCH v3 0/6] Make the qemu_logfile handle thread safe.
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(-) -- 2.17.1
Re: [PATCH 1/2] target/arm: Do not reject rt == rt2 for strexd
Richard Henderson writes: > > There was too much cut and paste between ldrexd and strexd, > as ldrexd does prohibit two output registers the same. > > Fixes: af288228995 > Reported-by: Michael Goffioul > Signed-off-by: Richard Henderson > --- Reviewed-by: Robert Foley
Re: [PATCH v1 5/5] tests/plugins: make howvec clean-up after itself.
On Fri, 7 Feb 2020 at 10:01, Alex Bennée wrote: > > TCG plugins are responsible for their own memory usage and although > the plugin_exit is tied to the end of execution in this case it is > still poor practice. Ensure we delete the hash table and related data > when we are done to be a good plugin citizen. > > Signed-off-by: Alex Bennée > --- > tests/plugin/howvec.c | 12 +++- > static void plugin_exit(qemu_plugin_id_t id, void *p) > { > g_autoptr(GString) report = g_string_new("Instruction Classes:\n"); > @@ -213,12 +220,15 @@ static void plugin_exit(qemu_plugin_id_t id, void *p) > g_list_free(it); > } > > +g_list_free(counts); > +g_hash_table_destroy(insns); > + Just one minor comment. Seems like there might be an option to use g_autoptr(GList) for counts. Reviewed-by: Robert Foley > qemu_plugin_outs(report->str); > } > > static void plugin_init(void) > { > -insns = g_hash_table_new(NULL, g_direct_equal); > +insns = g_hash_table_new_full(NULL, g_direct_equal, NULL, _record); > } > > static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata) > -- > 2.20.1 >
Re: [PATCH 1/2] test-logging: Fix -Werror=maybe-uninitialized warning
Good catch. Reviewed-by: Robert Foley On Tue, 21 Jan 2020 at 04:58, Thomas Huth wrote: > > On 21/01/2020 10.28, mreza...@redhat.com wrote: > > From: Miroslav Rezanina > > > > Checking for uninitialized variables raises warning for file path > > variables in test_logfile_write and test_logfile_lock functions. > > > > To suppress this warning, initialize varibles to NULL. This is safe > > change as result of g_build_filename is stored to them before any usage. > > > > Signed-off-by: Miroslav Rezanina > > --- > > tests/test-logging.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/tests/test-logging.c b/tests/test-logging.c > > index 1e646f0..6387e49 100644 > > --- a/tests/test-logging.c > > +++ b/tests/test-logging.c > > @@ -114,8 +114,8 @@ static void test_logfile_write(gconstpointer data) > > QemuLogFile *logfile2; > > gchar const *dir = data; > > Error *err = NULL; > > -g_autofree gchar *file_path; > > -g_autofree gchar *file_path1; > > +g_autofree gchar *file_path = NULL; > > +g_autofree gchar *file_path1 = NULL; > > FILE *orig_fd; > > > > /* > > @@ -157,7 +157,7 @@ static void test_logfile_lock(gconstpointer data) > > FILE *logfile; > > gchar const *dir = data; > > Error *err = NULL; > > -g_autofree gchar *file_path; > > +g_autofree gchar *file_path = NULL; > > > > file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL); > > Right. The glib documentation clearly states that "the variable must be > initialized", see: > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > > So this is the right thing to do here! > > Reviewed-by: Thomas Huth >
Re: [PATCH v2 00/14] tests/vm: Add support for aarch64 VMs
On Tue, 3 Mar 2020 at 05:24, Alex Bennée wrote: > > We are happy to make any adjustments here. Our first set of > > refactoring here was > > aimed at making it more pythonic. Is this where the concerns are? > > I'm just worried about the fragility of multiple steps in the chain of > io we are snooping on. That said Phillipe made a reasonable point that > other tools could be used - QMP for example would be the way to check > the status of the network connection before we trigger ssh rather than > the current busy-timeout approach. However that would result in more > complexity so if what works is stable...*shrug* OK. Makes sense. Thanks for explaining. > > >> However I ran into problems testing on my aarch64 box > >> because of a missing gen-iso-image which makes me think we need to add > >> some gating via configure for tools and libraries we need. > > > > Should we error out in configure if the tools and libraries needed to build > > the > > VMs are not there? > > Or maybe tolerate these dependencies not being there at configure time and > > provide an error later when someone tries to vm-build these VMs? > > We currently do both ;-) > > When we can detect at configure time and skip in make we do - see > tests/docker/Makefile.include and the compiler tests for tests/tcg. > However the acceptance tests current use runtime decorators in python to > detect as we go - but that test framework prints a summary and doesn't > exit -1 to the rest of make if it skips something. > > > Just curious which approach we should pursue here. > > Have a look at: > > Subject: [PATCH v1 00/10] testing/next updates (tweaks and re-greening) > Date: Mon, 2 Mar 2020 18:18:57 + > Message-Id: <20200302181907.32110-1-alex.ben...@linaro.org> > > and see what you think. Thanks for the details. It seems we should be able to use a similar approach of detecting in configure and then skip and inform in make, for the aarch64 VM dependencies, as well as the yaml python dependency. Will look into adding these. Thanks & Regards, -Rob
Re: [PATCH v3 08/19] tests/iotests: be a little more forgiving on the size test
On Tue, 25 Feb 2020 at 07:47, Alex Bennée wrote: > > At least on ZFS this was failing as 512 was less than or equal to 512. > I suspect the reason is additional compression done by ZFS and however > qemu-img gets the actual size. > > Loosen the criteria to make sure after is not bigger than before and > also dump the values in the report. > > Signed-off-by: Alex Bennée Reviewed-by: Robert Foley
Re: [PATCH v2 00/14] tests/vm: Add support for aarch64 VMs
On Mon, 2 Mar 2020 at 11:38, Alex Bennée wrote: > > > Robert Foley writes: > > > This is version 2 of the patch series to > > add support for aarch64 VMs. > > - Ubuntu 18.04 aarch64 VM > > - CentOS 8 aarch64 VM > > For now I've pulled the first 5 patches into testing/next as they are > obvious clean-ups. > > > tests/vm: Add workaround to consume console > > I still have concerns about this approach but I'm going to give it some > more testing. We are happy to make any adjustments here. Our first set of refactoring here was aimed at making it more pythonic. Is this where the concerns are? > However I ran into problems testing on my aarch64 box > because of a missing gen-iso-image which makes me think we need to add > some gating via configure for tools and libraries we need. Should we error out in configure if the tools and libraries needed to build the VMs are not there? Or maybe tolerate these dependencies not being there at configure time and provide an error later when someone tries to vm-build these VMs? Just curious which approach we should pursue here. Thanks & Regards, -Rob
Re: [PATCH 3/8] tests/vm: change wait_ssh to optionally wait for root.
On Mon, 27 Jan 2020 at 06:06, Alex Bennée wrote: > > Allow wait_ssh to wait for root user to be ready. > > This solves the issue where we perform a wait_ssh() > > successfully, but the root user is not yet ready > > to be logged in. > > So in the case it's the root user we care about... We care about both the root and guest users. See below. > > tests/vm/basevm.py | 9 +++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py > > index 86908f58ec..3b4403ddcb 100755 > > --- a/tests/vm/basevm.py > > +++ b/tests/vm/basevm.py > > @@ -310,12 +310,17 @@ class BaseVM(object): > > def print_step(self, text): > > sys.stderr.write("### %s ...\n" % text) > > > > -def wait_ssh(self, seconds=600): > > +def wait_ssh(self, wait_root=False, seconds=600): > > starttime = datetime.datetime.now() > > endtime = starttime + datetime.timedelta(seconds=seconds) > > guest_up = False > > while datetime.datetime.now() < endtime: > > -if self.ssh("exit 0") == 0: > > +if wait_root: > > +if self.ssh("exit 0") == 0 and\ > > + self.ssh_root("exit 0") == 0: > > ...why do we need to test both here? We want to make sure the root user is up in addition to the normal/guest user. We're trying to add on the root user since the issue we saw is that the guest user was up, (wait_ssh() completed), but then when the root user tries to do something we get an error, since root is not ready yet. > > +guest_up = True > > +break > > +elif self.ssh("exit 0") == 0: > > Is this simpler? Certainly simpler. :) And simpler seems like the right call here. But we'll need to call into wait_ssh() twice, once with the wait_root option and once without. But I think this is better since it makes the code on the caller side more explicit and clear in that we will explicitly wait for the guest user and then wait for the root user. Thanks, -Rob Foley > def wait_ssh(self, wait_root=False, seconds=600): > starttime = datetime.datetime.now() > endtime = starttime + datetime.timedelta(seconds=seconds) > guest_up = False > while datetime.datetime.now() < endtime: > if wait_root and self.ssh_root("exit 0") == 0: > guest_up = True > break > elif self.ssh("exit 0") == 0: > guest_up = True > break > seconds = (endtime - datetime.datetime.now()).total_seconds() > logging.debug("%ds before timeout", seconds) > time.sleep(1) > if not guest_up: > raise Exception("Timeout while waiting for guest ssh") > > > > guest_up = True > > break > > seconds = (endtime - datetime.datetime.now()).total_seconds() > > > -- > Alex Bennée
Re: [PATCH 0/8] tests/vm: Add support for aarch64 VMs
Thanks for the details on the failure. I have not been able to reproduce it yet, but digging into it further. On Tue, 28 Jan 2020 at 12:52, Alex Bennée wrote: > > > Robert Foley writes: > > > This patch adds support for 2 aarch64 VMs. > > - Ubuntu 18.04 aarch64 VM > > - CentOS 8 aarch64 VM > > > Another failure to note - under TCG: > > make vm-build-ubuntu.aarch64 V=1 QEMU=aarch64-softmmu/qemu-system-aarch64 > > Gives: > > Not run: 172 186 192 220 > Failures: 001 002 003 004 005 007 008 009 010 011 012 013 017 018 019 020 021 > 022 024 025 027 029 031 032 033 034 035 036 037 038 039 042 043 046 047 048 > 049 050 052 053 054 060 061 062 063 066 069 071 072 073 074 079 080 086 089 > 090 097 098 099 103 104 105 107 108 110 111 114 117 120 126 133 134 137 138 > 140 141 143 150 154 156 158 159 161 170 > 174 176 177 179 184 187 190 191 195 214 217 226 229 244 249 251 252 265 267 > 268 > Failed 104 of 104 iotests > /tmp/tmp.EjcqWtvHwd/tests/Makefile.include:840: recipe for target > 'check-tests/check-block.sh' failed > make: *** [check-tests/check-block.sh] Error 1 > rm tests/qemu-iotests/socket_scm_helper.o > Connection to 127.0.0.1 closed. > DEBUG:QMP:>>> {'execute': 'quit'} > DEBUG:QMP:<<< {'timestamp': {'seconds': 1580134315, 'microseconds': 216297}, > 'event': 'NIC_RX_FILTER_CHANGED', 'data': {'path': > '/machine/peripheral-anon/device[0]/virtio-backend'}} > DEBUG:QMP:<<< {'return': {}} > /home/alex.bennee/lsrc/qemu.git/tests/vm/Makefile.include:63: recipe for > target 'vm-build-ubuntu.aarch64' failed > make: *** [vm-build-ubuntu.aarch64] Error 3 > > With things like: > > --- /tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/063.out 2020-01-27 > 10:54:38.0 + > +++ /tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/063.out.bad 2020-01-28 > 01:20:28.563789323 + > @@ -1,3 +1,4 @@ > +bash: warning: setlocale: LC_ALL: cannot change locale (en_GB.UTF-8) > QA output created by 063 > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304 > == Testing conversion with -n fails with no target file == > TESTiotest-qcow2: 066 [fail] > QEMU -- > "/tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/../../aarch64-softmmu/qemu-system-aarch64" > -nodefaults -display none -machine virt -accel qtest > QEMU_IMG -- "/tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/../../qemu-img" > QEMU_IO -- "/tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/../../qemu-io" > --cache writeback -f qcow2 > QEMU_NBD -- "/tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/../../qemu-nbd" > IMGFMT-- qcow2 (compat=1.1) > IMGPROTO -- file > PLATFORM -- Linux/aarch64 ubuntu-guest 4.15.0-74-generic > TEST_DIR -- /tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/scratch > SOCK_DIR -- /tmp/tmp.BJ9gTNMmv1 > SOCKET_SCM_HELPER -- /tmp/tmp.EjcqWtvHwd/tests/qemu-iotests/socket_scm_helper > > So I suspect a locale issue is breaking things. > > -- > Alex Bennée
Re: [PATCH 7/8] tests/vm: Added a new script for ubuntu.aarch64.
Hi Drew, On Mon, 27 Jan 2020 at 12:27, Andrew Jones wrote: > > > > I suppose we could check the version of QEMU and use the above > > defaults only for earlier versions of QEMU. > > This is something we will probably move to aarch64vm.py since it is common. > > What versions of QEMU do these tests *have* to support? Because we could > just skip the tests for QEMU that doesn't support cpu=max,gic-version=max. > 'max' is indeed the nicest selection for using the same command line on > KVM (gicv2 and gicv3 hosts) and TCG. I believe these test scripts which build/launch the VM have to support the older version of QEMU since this is the version of QEMU currently used when these VMs are launched. I don't know the history on this, but it seems intentional that we use one older/different version of QEMU to launch the VM, while we test the 'current' build of QEMU inside the VM. It also seems like a 'nice to have' to automatically support the latest version where we could use max as you pointed out. Thanks & Regards, -Rob
Re: [PATCH 7/8] tests/vm: Added a new script for ubuntu.aarch64.
On Mon, 27 Jan 2020 at 15:07, Alex Bennée wrote: > Robert Foley writes: > > > Hi Drew, > > > > On Mon, 27 Jan 2020 at 12:27, Andrew Jones wrote: > > > >> > > >> > I suppose we could check the version of QEMU and use the above > >> > defaults only for earlier versions of QEMU. > >> > This is something we will probably move to aarch64vm.py since it is > >> > common. > >> > >> What versions of QEMU do these tests *have* to support? Because we could > >> just skip the tests for QEMU that doesn't support cpu=max,gic-version=max. > >> 'max' is indeed the nicest selection for using the same command line on > >> KVM (gicv2 and gicv3 hosts) and TCG. > > > > I believe these test scripts which build/launch the VM have to support > > the older version of QEMU since > > this is the version of QEMU currently used when these VMs are > > launched. I don't know the history on > > this, but it seems intentional that we use one older/different version > > of QEMU to launch the VM, > > Well we defer to the system QEMU as it should be stable. It can be > overridden with the QEMU environment variable which worked well enough > when we only had VMs of one architecture. Perhaps we needs a new way to > say "use the appropriate QEMU from this build"? Good idea. This is a pretty common use case and it makes sense to make it easy to use. I will add some support for this in my patch series. Thanks & Regards, -Rob > > > while we test the 'current' build of QEMU inside the VM. > > It also seems like a 'nice to have' to automatically support the > > latest version where we could > > use max as you pointed out. > > > > Thanks & Regards, > > -Rob > > > -- > Alex Bennée
Re: [PATCH 5/8] tests/vm: Added configuration file support
On Mon, 27 Jan 2020 at 07:38, Alex Bennée wrote: > > +if 'password' in target_dict: > > +config['root_pass'] = target_dict['password'] > > +config['guest_pass'] = target_dict['password'] > > This seems like impedance matching between two dictionaries. Surely it > would be nicer for the config to be read in fully formed and referable by > the rest of the code. We can also change the internal references. Good point. Will rework it to avoid this matching. Basically we can put the values we read directly into the config dictionary in one step. > > > +if any (k in target_dict for k in ("ssh_key","ssh_pub_key")) and \ > > + not all (k in target_dict for k in ("ssh_key","ssh_pub_key")): > > +missing_key = "ssh_pub_key" \ > > + if 'ssh_key' in target_dict else "ssh_key" > > +raise Exception("both ssh_key and ssh_pub_key required. " > > +"{} key is missing.".format(missing_key)) > > I guess validation has to be done at some time.. but > > > +if 'ssh_key' in target_dict: > > +config['ssh_key_file'] = target_dict['ssh_key'] > > +if not os.path.exists(config['ssh_key_file']): > > +raise Exception("ssh key file not found.") > > +if 'ssh_pub_key' in target_dict: > > +config['ssh_pub_key_file'] = target_dict['ssh_pub_key'] > > +if not os.path.exists(config['ssh_pub_key_file']): > > +raise Exception("ssh pub key file not found.") > > here we are both munging dictionaries again before checking the data. > Given we bail with an exception I'm now rethinking if it makes sense to > validate up here. It depends on how many places in the code expect to > use this data. Makes sense. We will change it to validate in one place just before we expect to use this data. > > +# By default we do not set the DNS. > > +# You override the defaults by setting the below. > > +#dns: 1.234.567.89 > > + > > +# By default we will use a "block" device, but > > +# you can also boot from a "scsi" device. > > +# Just keep in mind your scripts might need to change > > +# As you will have /dev/sda instead of /dev/vda (for block device) > > +#boot_dev_type: "scsi" > > + > > +# By default the ssh port is not fixed. > > +# A fixed ssh port makes it easier for automated tests. > > +#ssh_port: > > + > > +# To install a different set of packages, provide a command to issue > > +#install_cmds: "apt-get update ; apt-get build-dep -y qemu" > > + > > Having the example is great. It would be nice to see at least one of the > others converted to a config driven approach as well The example we provided was primarily for aarch64, we will add one or more examples here for the other VMs to help provide a ready to use template for providing a config file. > - is the config driven approach going to reduce duplication across the > various bits of > VM configuring python? Should everything be config driven? Are we in > danger of re-inventing an exiting tooling? All interesting questions to explore. Here is my take on this. One goal we had in mind is to not require a config file for any given VM. So in this sense we are not going in the direction of a config driven approach. Even the VMs that we added for aarch64 do not require a config file. The VM scripts will work as is without a config file since the script itself provides all required defaults. Our intention was for the config approach to be used to allow overriding the defaults for any given VM, to give the flexibility of overriding the parameters. But on the other hand by not requiring a config file, we make is simpler and easier to only override the parameters that the user is interested in. And also to limit the cases where we could generate a non-working VM if the user forgot to provide certain defaults. Thanks & Regards, -Rob
Re: [PATCH 7/8] tests/vm: Added a new script for ubuntu.aarch64.
On Mon, 27 Jan 2020 at 10:01, Alex Bennée wrote: > > vm-boot-ssh-%: $(IMAGES_DIR)/%.img > > $(call quiet-command, \ > > - $(SRC_PATH)/tests/vm/$* \ > > + $(PYTHON) $(SRC_PATH)/tests/vm/$* \ > > This seems like it should be in a different patch. Good point, will move it to a different patch. > > + > > +DEFAULT_CONFIG = { > > +'cpu' : "max", > > +'machine' : "virt,gic-version=max", > > According to virt.c: > > Valid values are 2, 3 and host > > but max should work on TCG. However we need a more recent QEMU than my > system one for it to work. Otherwise you see: > > DEBUG:qemu.machine:Error launching VM Good point. We were trying to avoid having different values for KVM vs TCG, which we could do with the latest QEMU. We will update this to make sure this works with older versions of QEMU as well. On my system I have qemu 2.11.1 installed by default. It seems that we will need the following defaults based on our environment. For KVM we end up with the below args since max cpu and max gic-version is not available. kvm: -cpu host -machine virt,gic-version=host For TCG max cpu is also not available: qemu-system-aarch64: unable to find CPU model 'max', so we pick cortex-a57. TCG: -cpu cortex-a57 -machine virt,gic-version=3 I suppose we could check the version of QEMU and use the above defaults only for earlier versions of QEMU. This is something we will probably move to aarch64vm.py since it is common. > > +class UbuntuAarch64VM(basevm.BaseVM): > > +name = "ubuntu.aarch64" > > +arch = "aarch64" > > +image_name = "ubuntu-18.04-server-cloudimg-arm64.img" > > +image_link = "https://cloud-images.ubuntu.com/releases/18.04/release/; > > + image_name > > +login_prompt = "ubuntu-guest login:" > > +BUILD_SCRIPT = """ > > +set -e; > > +cd $(mktemp -d); > > +sudo chmod a+r /dev/vdb; > > +tar --checkpoint=.10 -xf /dev/vdb; > > +./configure {configure_opts}; > > +make --output-sync {target} -j{jobs} {verbose}; > > +""" > > +def _gen_cloud_init_iso(self): __snip__ > > + > > +return os.path.join(cidir, "cloud-init.iso") > > It seems this function is proliferating. It certainly seems common > enough to be basevm functionality. Makes sense. Will look at making this common to basevm. > > > + > > +def boot(self, img, extra_args=None): > > +aarch64vm.create_flash_images() > > +default_args = aarch64vm.get_pflash_args() > > +if extra_args: > > +extra_args.extend(default_args) > > +else: > > +extra_args = default_args > > +# We always add these performance tweaks > > +# because without them, we boot so slowly that we > > +# can time out finding the boot efi device. > > +if os.geteuid() != 0: > > +extra_args.extend(["-accel", "tcg,thread=multi"]) > > Hmmm thread=multi should already be enabled by default where it is safe > to do so. Also what has it to do with euid? OK. Will look into removing this. We were trying to check for KVM, to only add this under KVM. I see now, we need to use kvm_available() instead of euid. Thanks & Regards, -Rob > > > +if '-smp' not in extra_args and \ > > + '-smp' not in self._config['extra_args'] and \ > > + '-smp' not in self._args: > > +# Only add if not already there to give caller option to > > change it. > > +extra_args.extend(["-smp", "8"]) > > + > > +# We have overridden boot() since aarch64 has additional > > parameters. > > +# Call down to the base class method. > > +super(UbuntuAarch64VM, self).boot(img, extra_args=extra_args) > > + > > +def build_image(self, img): > > +os_img = self._download_with_cache(self.image_link) > > +img_tmp = img + ".tmp" > > +subprocess.check_call(["cp", "-f", os_img, img_tmp]) > > +subprocess.check_call(["qemu-img", "resize", img_tmp, "+50G"]) > > +ci_img = self._gen_cloud_init_iso() > > + > > +self.boot(img_tmp, extra_args = ["-cdrom", ci_img]) > > +self.wait_ssh(wait_root=True) > > +# Fix for slow ssh login. > > +self.ssh_root("chmod -x /etc/update-motd.d/*") > > +self.ssh_root("touch /etc/cloud/cloud-init.disabled") > > +# Disable auto upgrades. > > +# We want to keep the VM system state stable. > > +self.ssh_root('sed -ie \'s/"1"/"0"/g\' > > /etc/apt/apt.conf.d/20auto-upgrades') > > + > > +# If the user chooses *not* to do the second phase, > > +# then we will jump right to the graceful shutdown > > +if self._config['install_cmds'] != "": > > +# Don't check the status in case the guest hang up too quickly > > +self.ssh_root("sync && reboot") > > + > > +self.wait_ssh(wait_root=True) > > +# The previous update sometimes doesn't survive a reboot, so
[PATCH v1 03/14] tests/vm: increased max timeout for vm boot.
Add change to increase timeout waiting for VM to boot. Needed for some emulation cases where it can take longer than 5 minutes to boot. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 7 +++ 1 file changed, 7 insertions(+) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 991115e44b..4de358ae22 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -59,6 +59,10 @@ class BaseVM(object): poweroff = "poweroff" # enable IPv6 networking ipv6 = True +# Scale up some timeouts under TCG. +# 4 is arbitrary, but greater than 2, +# since we found we need to wait more than twice as long. +tcg_ssh_timeout_multiplier = 4 def __init__(self, debug=False, vcpus=None): self._guest = None self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", @@ -311,6 +315,9 @@ class BaseVM(object): sys.stderr.write("### %s ...\n" % text) def wait_ssh(self, seconds=300): +# Allow more time for VM to boot under TCG. +if not kvm_available(self.arch): +seconds *= self.tcg_ssh_timeout_multiplier starttime = datetime.datetime.now() endtime = starttime + datetime.timedelta(seconds=seconds) guest_up = False -- 2.17.1
[PATCH v1 08/14] tests/vm: Added configuration file support
Changes to tests/vm/basevm.py to allow accepting a configuration file as a parameter. Allows for specifying VM options such as cpu, machine, memory, and arbitrary qemu arguments for specifying options such as NUMA configuration. Also added an example conf_example_aarch64.yml and conf_example_x86.yml. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 2 ++ tests/vm/basevm.py| 29 +- tests/vm/conf_example_aarch64.yml | 51 +++ tests/vm/conf_example_x86.yml | 50 ++ 4 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 tests/vm/conf_example_aarch64.yml create mode 100644 tests/vm/conf_example_x86.yml diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 778e506755..e9ed33226d 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -35,6 +35,8 @@ vm-help vm-test: @echo "V=1 - Enable verbose ouput on host and guest commands" @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool" + @echo "QEMU_CONFIG=/path/conf.yml - Change path to VM configuration .yml file." + @echo " See conf_example_*.yml for file format details." vm-build-all: $(addprefix vm-build-, $(IMAGES)) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 33004934af..f488d4103c 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -32,6 +32,7 @@ import shutil import multiprocessing import traceback from socket_thread import SocketThread +import yaml SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), "..", "keys", "id_rsa") @@ -500,9 +501,31 @@ class BaseVM(object): cwd=cidir, stdin=self._devnull, stdout=self._stdout, stderr=self._stdout) - return os.path.join(cidir, "cloud-init.iso") +def parse_config(config, args): +""" Parse yaml config and populate our config structure. +The yaml config allows the user to override the +defaults for VM parameters. In many cases these +defaults can be overridden without rebuilding the VM.""" +if args.config: +config_file = args.config +elif 'QEMU_CONFIG' in os.environ: +config_file = os.environ['QEMU_CONFIG'] +else: +return config +if not os.path.exists(config_file): +raise Exception("config file {} does not exist".format(config_file)) +with open(config_file) as f: +yaml_dict = yaml.safe_load(f) + +if 'qemu-conf' in yaml_dict: +config.update(yaml_dict['qemu-conf']) +else: +raise Exception("config file {} is not valid"\ +" missing qemu-conf".format(config_file)) +return config + def parse_args(vmcls): def get_default_jobs(): @@ -537,6 +560,9 @@ def parse_args(vmcls): help="Interactively run command") parser.add_option("--snapshot", "-s", action="store_true", help="run tests with a snapshot") +parser.add_option("--config", "-c", default=None, + help="Provide config yaml for configuration. "\ + "See config_example.yaml for example.") parser.disable_interspersed_args() return parser.parse_args() @@ -548,6 +574,7 @@ def main(vmcls, config=None): if not argv and not args.build_qemu and not args.build_image: print("Nothing to do?") return 1 +config = parse_config(config, args) logging.basicConfig(level=(logging.DEBUG if args.debug else logging.WARN)) vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config) diff --git a/tests/vm/conf_example_aarch64.yml b/tests/vm/conf_example_aarch64.yml new file mode 100644 index 00..9d44ae356f --- /dev/null +++ b/tests/vm/conf_example_aarch64.yml @@ -0,0 +1,51 @@ +# +# Example yaml for use by any of the scripts in tests/vm. +# Can be provided as an environment variable QEMU_CONFIG +# +qemu-conf: + +# If any of the below are not provided, we will just use the qemu defaults. + +# Login username and password(has to be sudo enabled) +guest_user: qemu +guest_pass: "qemupass" + +# Password for root user can be different from guest. +root_pass: "qemupass" + +# If one key is provided, both must be provided. +#ssh_key: /complete/path/of/your/keyfile/id_rsa +#ssh_pub_key: /complete/path/of/your/keyfile/id_rsa.pub + +
[PATCH v1 14/14] tests/vm: change scripts to use self._config
This change converts existing scripts to using for example self.ROOT_PASS, to self._config['root_pass']. We made similar changes for GUEST_USER, and GUEST_PASS. This allows us also to remove the change in basevm.py, which adds __getattr__ for backwards compatibility. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 11 ++- tests/vm/fedora| 17 + tests/vm/freebsd | 16 tests/vm/netbsd| 19 ++- tests/vm/openbsd | 17 + 5 files changed, 38 insertions(+), 42 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index dc975d92c7..d2c5d32f34 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -184,13 +184,6 @@ class BaseVM(object): self.console_init(timeout=timeout) self.console_wait(wait_string) -def __getattr__(self, name): -# Support direct access to config by key. -# for example, access self._config['cpu'] by self.cpu -if name.lower() in self._config.keys(): -return self._config[name.lower()] -return object.__getattribute__(self, name) - def _download_with_cache(self, url, sha256sum=None, sha512sum=None): def check_sha256sum(fname): if not sha256sum: @@ -240,13 +233,13 @@ class BaseVM(object): return r def ssh(self, *cmd): -return self._ssh_do(self.GUEST_USER, cmd, False) +return self._ssh_do(self._config["guest_user"], cmd, False) def ssh_root(self, *cmd): return self._ssh_do("root", cmd, False) def ssh_check(self, *cmd): -self._ssh_do(self.GUEST_USER, cmd, True) +self._ssh_do(self._config["guest_user"], cmd, True) def ssh_root_check(self, *cmd): self._ssh_do("root", cmd, True) diff --git a/tests/vm/fedora b/tests/vm/fedora index 8e270fc0f0..4616d16740 100755 --- a/tests/vm/fedora +++ b/tests/vm/fedora @@ -105,20 +105,20 @@ class FedoraVM(basevm.BaseVM): self.console_wait_send("7) [!] Root password", "7\n") self.console_wait("Password:") -self.console_send("%s\n" % self.ROOT_PASS) +self.console_send("%s\n" % self._config["root_pass"]) self.console_wait("Password (confirm):") -self.console_send("%s\n" % self.ROOT_PASS) +self.console_send("%s\n" % self._config["root_pass"]) self.console_wait_send("8) [ ] User creation", "8\n") self.console_wait_send("1) [ ] Create user", "1\n") self.console_wait_send("3) User name", "3\n") -self.console_wait_send("ENTER:", "%s\n" % self.GUEST_USER) +self.console_wait_send("ENTER:", "%s\n" % self._config["guest_user"]) self.console_wait_send("4) [ ] Use password", "4\n") self.console_wait_send("5) Password", "5\n") self.console_wait("Password:") -self.console_send("%s\n" % self.GUEST_PASS) +self.console_send("%s\n" % self._config["guest_pass"]) self.console_wait("Password (confirm):") -self.console_send("%s\n" % self.GUEST_PASS) +self.console_send("%s\n" % self._config["guest_pass"]) self.console_wait_send("7) Groups","c\n") while True: @@ -136,7 +136,7 @@ class FedoraVM(basevm.BaseVM): if good: break time.sleep(10) -self.console_send("r\n" % self.GUEST_PASS) +self.console_send("r\n" % self._config["guest_pass"]) self.console_wait_send("'b' to begin install", "b\n") @@ -147,12 +147,13 @@ class FedoraVM(basevm.BaseVM): # setup qemu user prompt = " ~]$" -self.console_ssh_init(prompt, self.GUEST_USER, self.GUEST_PASS) +self.console_ssh_init(prompt, self._config["guest_user"], + self._config["guest_pass"]) self.console_wait_send(prompt, "exit\n") # setup root user prompt = " ~]#" -self.console_ssh_init(prompt, "root", self.ROOT_PASS) +self.console_ssh_init(prompt, "root", self._config["root_pass"]) self.console_sshd_config(prompt) # setup virtio-blk #1 (tarfile) diff --git a/tests/vm/freebsd b/tests/vm/freebsd index 33a736298a..fd1f595aa9 100755 --- a/tests/vm/freebsd +++ b/tests/vm/freebsd @@ -110,9 +110,9 @@ class FreeBSDVM(basevm.BaseVM): # post-install configuration
[PATCH v1 11/14] tests/vm: allow wait_ssh() to specify command
This allows for waiting for completion of arbitrary commands. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 97d55f8030..ebedbce4ae 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -433,24 +433,24 @@ class BaseVM(object): def print_step(self, text): sys.stderr.write("### %s ...\n" % text) -def wait_ssh(self, wait_root=False, seconds=300): +def wait_ssh(self, wait_root=False, seconds=300, cmd="exit 0"): # Allow more time for VM to boot under TCG. if not kvm_available(self.arch): seconds *= self.tcg_ssh_timeout_multiplier starttime = datetime.datetime.now() endtime = starttime + datetime.timedelta(seconds=seconds) -guest_up = False +cmd_success = False while datetime.datetime.now() < endtime: -if wait_root and self.ssh_root("exit 0") == 0: -guest_up = True +if wait_root and self.ssh_root(cmd) == 0: +cmd_success = True break -elif self.ssh("exit 0") == 0: -guest_up = True +elif self.ssh(cmd) == 0: +cmd_success = True break seconds = (endtime - datetime.datetime.now()).total_seconds() logging.debug("%ds before timeout", seconds) time.sleep(1) -if not guest_up: +if not cmd_success: raise Exception("Timeout while waiting for guest ssh") def shutdown(self): -- 2.17.1
[PATCH v1 09/14] tests/vm: add --boot-console switch
Added ability to view console during boot via --boot-console switch to basevm.py. This helps debug issues that occur during the boot sequence. Also added a new special variable to vm-build: BOOT_CONSOLE=1 will cause this new --boot-console switch to be set. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 4 tests/vm/basevm.py| 11 +-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index e9ed33226d..d72babd5bf 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -32,6 +32,7 @@ vm-help vm-test: @echo 'EXTRA_CONFIGURE_OPTS="..."' @echo "J=[0..9]* - Override the -jN parameter for make commands" @echo "DEBUG=1 - Enable verbose output on host and interactive debugging" + @echo "BOOT_CONSOLE=1- Show the console output at boot time. " @echo "V=1 - Enable verbose ouput on host and guest commands" @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool" @@ -50,6 +51,7 @@ $(IMAGES_DIR)/%.img: $(SRC_PATH)/tests/vm/% \ $(call quiet-command, \ $(PYTHON) $< \ $(if $(V)$(DEBUG), --debug) \ + $(if $(BOOT_CONSOLE),--boot-console) \ --image "$@" \ --force \ --build-image $@, \ @@ -64,6 +66,7 @@ vm-build-%: $(IMAGES_DIR)/%.img $(if $(DEBUG), --interactive) \ $(if $(J),--jobs $(J)) \ $(if $(V),--verbose) \ + $(if $(BOOT_CONSOLE),--boot-console) \ --image "$<" \ $(if $(BUILD_TARGET),--build-target $(BUILD_TARGET)) \ --snapshot \ @@ -84,6 +87,7 @@ vm-boot-ssh-%: $(IMAGES_DIR)/%.img $(call quiet-command, \ $(PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(J),--jobs $(J)) \ + $(if $(BOOT_CONSOLE),--boot-console) \ --image "$<" \ --interactive \ false, \ diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index f488d4103c..b99ab0f20a 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -96,9 +96,11 @@ class BaseVM(object): # since we found we need to wait more than twice as long. tcg_ssh_timeout_multiplier = 4 console_logfile = "console.log" -def __init__(self, debug=False, vcpus=None, config=None): +def __init__(self, debug=False, vcpus=None, config=None, +boot_console=None): self._guest = None self._console_fd = None +self._boot_console = boot_console self._socket_thread = None self._console_timeout_sec = self.socket_timeout # Allow input config to override defaults. @@ -563,6 +565,8 @@ def parse_args(vmcls): parser.add_option("--config", "-c", default=None, help="Provide config yaml for configuration. "\ "See config_example.yaml for example.") +parser.add_option("--boot-console", action="store_true", + help="Show console during boot. ") parser.disable_interspersed_args() return parser.parse_args() @@ -577,7 +581,8 @@ def main(vmcls, config=None): config = parse_config(config, args) logging.basicConfig(level=(logging.DEBUG if args.debug else logging.WARN)) -vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config) +vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config, + boot_console=args.boot_console) if args.build_image: if os.path.exists(args.image) and not args.force: sys.stderr.writelines(["Image file exists: %s\n" % args.image, @@ -597,6 +602,8 @@ def main(vmcls, config=None): if args.snapshot: img += ",snapshot=on" vm.boot(img) +if vm._boot_console: +vm.wait_boot() vm.wait_ssh() except Exception as e: if isinstance(e, SystemExit) and e.code == 0: -- 2.17.1
[PATCH v1 06/14] tests/vm: Add logging of console to file.
This adds logging of the char device used by the console to a file. The basevm.py then uses this file to read chars from the console. One reason to add this is to aid with debugging. But another is because there is an issue where the QEMU might hang if the characters from the character device are not consumed by the script. Signed-off-by: Robert Foley --- tests/vm/basevm.py| 48 ++--- tests/vm/socket_thread.py | 73 +++ 2 files changed, 116 insertions(+), 5 deletions(-) create mode 100644 tests/vm/socket_thread.py diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index a926211da8..87a484c55c 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -31,12 +31,17 @@ import tempfile import shutil import multiprocessing import traceback +from socket_thread import SocketThread SSH_KEY = open(os.path.join(os.path.dirname(__file__), "..", "keys", "id_rsa")).read() SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), "..", "keys", "id_rsa.pub")).read() +class ConsoleTimeoutException(Exception): +"""Raise this exception when read from console times out.""" +pass + class BaseVM(object): GUEST_USER = "qemu" GUEST_PASS = "qemupass" @@ -59,12 +64,18 @@ class BaseVM(object): poweroff = "poweroff" # enable IPv6 networking ipv6 = True +# This is the timeout on the wait for console bytes. +socket_timeout = 120 # Scale up some timeouts under TCG. # 4 is arbitrary, but greater than 2, # since we found we need to wait more than twice as long. tcg_ssh_timeout_multiplier = 4 +console_logfile = "console.log" def __init__(self, debug=False, vcpus=None): self._guest = None +self._console_fd = None +self._socket_thread = None +self._console_timeout_sec = self.socket_timeout self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", suffix=".tmp", dir=".")) @@ -179,6 +190,15 @@ class BaseVM(object): "-device", "virtio-blk,drive=%s,serial=%s,bootindex=1" % (name, name)] +def init_console(self, socket): +"""Init the thread to dump console to a file. + Also open the file descriptor we will use to + read from the console.""" +self._socket_thread = SocketThread(socket, self.console_logfile) +self._console_fd = open(self.console_logfile, "r") +self._socket_thread.start() +print("console logfile is: {}".format(self.console_logfile)) + def boot(self, img, extra_args=[]): args = self._args + [ "-device", "VGA", @@ -201,6 +221,7 @@ class BaseVM(object): raise atexit.register(self.shutdown) self._guest = guest +self.init_console(guest.console_socket) usernet_info = guest.qmp("human-monitor-command", command_line="info usernet") self.ssh_port = None @@ -212,9 +233,10 @@ class BaseVM(object): raise Exception("Cannot find ssh port from 'info usernet':\n%s" % \ usernet_info) -def console_init(self, timeout = 120): -vm = self._guest -vm.console_socket.settimeout(timeout) +def console_init(self, timeout = None): +if timeout == None: +timeout = self.socket_timeout +self._console_timeout_sec = timeout def console_log(self, text): for line in re.split("[\r\n]", text): @@ -230,13 +252,27 @@ class BaseVM(object): # log console line sys.stderr.write("con recv: %s\n" % line) +def console_recv(self, n): +"""Read n chars from the console_logfile being dumped to + by the socket thread we created earlier.""" +start_time = time.time() +while True: +data = self._console_fd.read(1) +if data != "": +break +time.sleep(0.1) +elapsed_sec = time.time() - start_time +if elapsed_sec > self._console_timeout_sec: +raise ConsoleTimeoutException +return data.encode('latin1') + def console_wait(self, expect, expectalt = None): vm = self._guest output = "" while True: try: -chars = vm.console_socket.recv(1) -except socket.timeout: +chars = self.console_recv(1) +
[PATCH v1 00/14] tests/vm: Add support for aarch64 VMs
This is version 1 of the patch series to add support for 2 aarch64 VMs. - Ubuntu 18.04 aarch64 VM - CentOS 8 aarch64 VM Changes in version 1 - Added environment variable QEMU_LOCAL=1 which means use the appropriate QEMU binary from the current build. - Improved support of aarch64 VMs to be backwards compatible by default with older versions of QEMU. - Fixed a locale issue with ubuntu.aarch64 VM. The issue here was that we were not waiting for the cloud-init to finish, prior to rebooting the VM. - Workaround of issue where the test hangs as QEMU is waiting for the consumption of character device data. Workaround adds a thread to the test framework to consume these characters and dump them to a file. The test then consumes these characters from the file instead of from the character device as it did previously. One advantage of this change is that there is now a log file which contains the console output for debug. - Change creation of aarch64 flash files to be in temp directory. - Added changes to convert existing scripts to using self._config for SSH_KEY_PUB, GUEST_USER, GUEST_PASS, ROOT_PASS. - Created example config file for x86, renamed existing example file to aarch64. - Make gen_cloud_init_iso() common to basevm. - Updated wait_ssh() to only extend time when we are under TCG. - Changed wait_ssh() to wait for root or wait for guest user, not both. - Removed SSH_KEY global as it is no longer needed. - Clarified use of ssh_key_file and ssh_pub_key_file, added tmp to the names to clarify, and added a comment to explain use here. - Cleaned up handling of the boot_console option. - Moved tweak to Makefile.include to a separate patch. - Removed setting of tcg,thread=multi in aarch64 VMs. - Moved the validation of ssh keys to a common location. - Reworked the handling of the config file yaml to simply set the values into the config dictionary in one step. More details on the patch in general: In order to add support for the two new aarch64 VMs, we generalized and parameterized basevm.py. We added a new concept of a configuration, which is really just a set of parameters which define how to configure the VM. Some examples of parameters are "machine", "memory" and "cpu". We preserved current default parameters. Current configuration of pre-existing VMs is supported by default without need to override default parameters. For example, previously only the 'pc' machine was supported in basevm.py. The new aarch64 VMs will override machine to use virt. There are a few other examples where we needed to add parameters in order to add support for these aarch64 VMs. In some other cases we added parameters that we thought would be useful in general, for example username/password, ssh keys, In the case of the aarch64 VMs, they override certain parameters by default. However, it is also of value to be able to dynamically specify other values for these parameters. Take the case where you create a new VM using vm-build, but then want to test it using various hardware configurations such as for example different NUMA topologies. Or maybe you want to use a different amount of memory or a different cpu type. In order to support these use cases we added support for a configuration .yml file, which allows the user to specify certain values dynamically such as: - machine - cpu - memory size - other qemu args, which allow configuring alternate hardware topologies such as NUMA nodes. - username, password - ssh keys For an example of a .yml file, see the included config_example.yml The main use case for using this config.yml file is for where we are testing/debugging with qemu (vm-build), and need to configure the VM differently. However, there is another use case we have developed, which is a project called lisa-qemu (https://github.com/rf972/lisa-qemu). This project is an integration between the LISA tool and QEMU. This project uses the VMs created by QEMU's vm-build scripts for use in testing with LISA. This use case is similar to the vm-build case in that, the VM gets created once, and we want to launch the VM with different configurations (memory, cpu, etc.). As part of developing the scripts for these VMs, we implemented a few enhancements to help with testing. For example, we added support for allowing debug mode to show the ssh output. We also added support for a new --boot-console option which will show the boot console as the VM boots up to aid in debugging problems during VM boot. Robert Foley (14): tests/vm: use $(PYTHON) consistently tests/vm: Debug mode shows ssh output. tests/vm: increased max timeout for vm boot. tests/vm: give wait_ssh() option to wait for root tests/vm: Added gen_cloud_init_iso() to basevm.py tests/vm: Add logging of console to file. tests/vm: Add configuration to basevm.py tests/vm: Added configuration file support tests/vm: add --boot-console switch tests/vm: Add abili
[PATCH v1 04/14] tests/vm: give wait_ssh() option to wait for root
Allow wait_ssh to wait for root user to be ready. This solves the issue where we perform a wait_ssh() successfully, but the root user is not yet ready to be logged in. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 4de358ae22..a29099f6f1 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -314,7 +314,7 @@ class BaseVM(object): def print_step(self, text): sys.stderr.write("### %s ...\n" % text) -def wait_ssh(self, seconds=300): +def wait_ssh(self, wait_root=False, seconds=300): # Allow more time for VM to boot under TCG. if not kvm_available(self.arch): seconds *= self.tcg_ssh_timeout_multiplier @@ -322,7 +322,10 @@ class BaseVM(object): endtime = starttime + datetime.timedelta(seconds=seconds) guest_up = False while datetime.datetime.now() < endtime: -if self.ssh("exit 0") == 0: +if wait_root and self.ssh_root("exit 0") == 0: +guest_up = True +break +elif self.ssh("exit 0") == 0: guest_up = True break seconds = (endtime - datetime.datetime.now()).total_seconds() @@ -333,7 +336,6 @@ class BaseVM(object): def shutdown(self): self._guest.shutdown() - def wait(self): self._guest.wait() -- 2.17.1
[PATCH v1 07/14] tests/vm: Add configuration to basevm.py
Added use of a configuration to tests/vm/basevm.py. The configuration provides parameters used to configure a VM. This allows for providing alternate configurations to the VM being created/launched. cpu, machine, memory, and NUMA configuration are all examples of configuration which we might want to vary on the VM being created or launched. This will for example allow for creating an aarch64 vm. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 141 +++-- 1 file changed, 111 insertions(+), 30 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 87a484c55c..33004934af 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -33,19 +33,43 @@ import multiprocessing import traceback from socket_thread import SocketThread -SSH_KEY = open(os.path.join(os.path.dirname(__file__), - "..", "keys", "id_rsa")).read() -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), - "..", "keys", "id_rsa.pub")).read() +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), + "..", "keys", "id_rsa") +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), + "..", "keys", "id_rsa.pub") class ConsoleTimeoutException(Exception): """Raise this exception when read from console times out.""" pass +# This is the standard configuration. +# Any or all of these can be overridden by +# passing in a config argument to the VM constructor. +DEFAULT_CONFIG = { +'cpu' : "max", +'machine' : 'pc', +'guest_user' : "qemu", +'guest_pass' : "qemupass", +'root_pass' : "qemupass", +'ssh_key_file': SSH_KEY_FILE, +'ssh_pub_key_file': SSH_PUB_KEY_FILE, +'memory' : "4G", +'extra_args' : [], +'qemu_args' : "", +'dns' : "", +'ssh_port': 0, +'install_cmds': "", +'boot_dev_type' : "block", +'ssh_timeout' : 1, +} +BOOT_DEVICE = { +'block' : "-drive file={},if=none,id=drive0,cache=writeback "\ + "-device virtio-blk,drive=drive0,bootindex=0", +'scsi' : "-device virtio-scsi-device,id=scsi "\ + "-drive file={},format=raw,if=none,id=hd0 "\ + "-device scsi-hd,drive=hd0,bootindex=0", +} class BaseVM(object): -GUEST_USER = "qemu" -GUEST_PASS = "qemupass" -ROOT_PASS = "qemupass" envvars = [ "https_proxy", @@ -71,22 +95,33 @@ class BaseVM(object): # since we found we need to wait more than twice as long. tcg_ssh_timeout_multiplier = 4 console_logfile = "console.log" -def __init__(self, debug=False, vcpus=None): +def __init__(self, debug=False, vcpus=None, config=None): self._guest = None self._console_fd = None self._socket_thread = None self._console_timeout_sec = self.socket_timeout +# Allow input config to override defaults. +self._config = DEFAULT_CONFIG.copy() +if config != None: +self._config.update(config) +self.validate_ssh_keys() self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", suffix=".tmp", dir=".")) atexit.register(shutil.rmtree, self._tmpdir) - -self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") -open(self._ssh_key_file, "w").write(SSH_KEY) -subprocess.check_call(["chmod", "600", self._ssh_key_file]) - -self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") -open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY) +# Copy the key files to a temporary directory. +# Also chmod the key file to agree with ssh requirements. +self._config['ssh_key'] = \ +open(self._config['ssh_key_file']).read().rstrip() +self._config['ssh_pub_key'] = \ +open(self._config['ssh_pub_key_file']).read().rstrip() +self._ssh_tmp_key_file = os.path.join(self._tmpdir, "id_rsa") +open(self._ssh_tmp_key_file, "w").write(self._config['ssh_key']) +subprocess.check_call(["chmod", "600", self._ssh_tmp_key_file]) + +self._ssh_tmp_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") +open(self._ssh_tmp_pub_key_file, + "w").write(self._config['ssh_pub_key']) self.debug = debug self._stder
[PATCH v1 02/14] tests/vm: Debug mode shows ssh output.
Add changes to tests/vm/basevm.py so that during debug mode we show ssh output. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov Reviewed-by: Alex Bennée --- tests/vm/basevm.py | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index ed5dd4f3d0..991115e44b 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -122,11 +122,16 @@ class BaseVM(object): return fname def _ssh_do(self, user, cmd, check): -ssh_cmd = ["ssh", "-q", "-t", +ssh_cmd = ["ssh", + "-t", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=" + os.devnull, "-o", "ConnectTimeout=1", "-p", self.ssh_port, "-i", self._ssh_key_file] +# If not in debug mode, set ssh to quiet mode to +# avoid printing the results of commands. +if not self.debug: +ssh_cmd.append("-q") for var in self.envvars: ssh_cmd += ['-o', "SendEnv=%s" % var ] assert not isinstance(cmd, str) -- 2.17.1
[PATCH v1 05/14] tests/vm: Added gen_cloud_init_iso() to basevm.py
This method was located in both centos and ubuntu.i386. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 40 tests/vm/centos | 33 + tests/vm/ubuntu.i386 | 37 + 3 files changed, 42 insertions(+), 68 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index a29099f6f1..a926211da8 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -346,6 +346,46 @@ class BaseVM(object): def qmp(self, *args, **kwargs): return self._guest.qmp(*args, **kwargs) +def gen_cloud_init_iso(self): +cidir = self._tmpdir +mdata = open(os.path.join(cidir, "meta-data"), "w") +name = self.name.replace(".","-") +mdata.writelines(["instance-id: {}-vm-0\n".format(name), + "local-hostname: {}-guest\n".format(name)]) +mdata.close() +udata = open(os.path.join(cidir, "user-data"), "w") +print("guest user:pw {}:{}".format(self._config['guest_user'], + self._config['guest_pass'])) +udata.writelines(["#cloud-config\n", + "chpasswd:\n", + " list: |\n", + "root:%s\n" % self._config['root_pass'], + "%s:%s\n" % (self._config['guest_user'], + self._config['guest_pass']), + " expire: False\n", + "users:\n", + " - name: %s\n" % self._config['guest_user'], + "sudo: ALL=(ALL) NOPASSWD:ALL\n", + "ssh-authorized-keys:\n", + "- %s\n" % self._config['ssh_pub_key'], + " - name: root\n", + "ssh-authorized-keys:\n", + "- %s\n" % self._config['ssh_pub_key'], + "locale: en_US.UTF-8\n"]) +proxy = os.environ.get("http_proxy") +if not proxy is None: +udata.writelines(["apt:\n", + " proxy: %s" % proxy]) +udata.close() +subprocess.check_call(["genisoimage", "-output", "cloud-init.iso", + "-volid", "cidata", "-joliet", "-rock", + "user-data", "meta-data"], + cwd=cidir, + stdin=self._devnull, stdout=self._stdout, + stderr=self._stdout) + +return os.path.join(cidir, "cloud-init.iso") + def parse_args(vmcls): def get_default_jobs(): diff --git a/tests/vm/centos b/tests/vm/centos index f2f0befd84..c108bd6799 100755 --- a/tests/vm/centos +++ b/tests/vm/centos @@ -31,37 +31,6 @@ class CentosVM(basevm.BaseVM): make docker-test-mingw@fedora {verbose} J={jobs} NETWORK=1; """ -def _gen_cloud_init_iso(self): -cidir = self._tmpdir -mdata = open(os.path.join(cidir, "meta-data"), "w") -mdata.writelines(["instance-id: centos-vm-0\n", - "local-hostname: centos-guest\n"]) -mdata.close() -udata = open(os.path.join(cidir, "user-data"), "w") -udata.writelines(["#cloud-config\n", - "chpasswd:\n", - " list: |\n", - "root:%s\n" % self.ROOT_PASS, - "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS), - " expire: False\n", - "users:\n", - " - name: %s\n" % self.GUEST_USER, - "sudo: ALL=(ALL) NOPASSWD:ALL\n", - "ssh-authorized-keys:\n", - "- %s\n" % basevm.SSH_PUB_KEY, - " - name: root\n", - "ssh-authorized-keys:\n", - "- %s\n" % basevm.SSH_PUB_KEY, - "locale: en_US.UTF-8\n"]) -udata.close() -subprocess.check_call(["genisoimage", "-output", "cloud-init.iso", - "-volid", "cidata", "-joliet", &quo
[PATCH v1 01/14] tests/vm: use $(PYTHON) consistently
Change Makefile.include to use $(PYTHON) so for vm-boot-ssh to be consistent with other cases like vm-build. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 9e7c46a473..778e506755 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -80,7 +80,7 @@ vm-boot-serial-%: $(IMAGES_DIR)/%.img vm-boot-ssh-%: $(IMAGES_DIR)/%.img $(call quiet-command, \ - $(SRC_PATH)/tests/vm/$* \ + $(PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(J),--jobs $(J)) \ --image "$<" \ --interactive \ -- 2.17.1
[PATCH v1 13/14] tests/vm: Added a new script for centos.aarch64.
centos.aarch64 creates a CentOS 8 image. Also added a new kickstart script used to build the centos.aarch64 image. Signed-off-by: Robert Foley --- tests/vm/Makefile.include| 3 +- tests/vm/centos-8-aarch64.ks | 51 tests/vm/centos.aarch64 | 221 +++ 3 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 tests/vm/centos-8-aarch64.ks create mode 100755 tests/vm/centos.aarch64 diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index ccafe966cd..7b65958f4d 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -2,7 +2,7 @@ .PHONY: vm-build-all vm-clean-all -IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 centos.aarch64 IMAGES_DIR := $(HOME)/.cache/qemu-vm/images IMAGE_FILES := $(patsubst %, $(IMAGES_DIR)/%.img, $(IMAGES)) @@ -19,6 +19,7 @@ vm-help vm-test: @echo " vm-build-centos - Build QEMU in CentOS VM, with Docker" @echo " vm-build-fedora - Build QEMU in Fedora VM" @echo " vm-build-ubuntu.aarch64 - Build QEMU in ubuntu aarch64 VM" + @echo " vm-build-centos.aarch64 - Build QEMU in CentOS aarch64 VM" @echo "" @echo " vm-build-all- Build QEMU in all VMs" @echo " vm-clean-all- Clean up VM images" diff --git a/tests/vm/centos-8-aarch64.ks b/tests/vm/centos-8-aarch64.ks new file mode 100644 index 00..fd6ebe4d49 --- /dev/null +++ b/tests/vm/centos-8-aarch64.ks @@ -0,0 +1,51 @@ +# CentOS aarch64 image kickstart file. +# This file is used by the CentOS installer to +# script the generation of the image. +# +# Copyright 2020 Linaro +# +ignoredisk --only-use=vda +# System bootloader configuration +bootloader --append=" crashkernel=auto" --location=mbr --boot-drive=vda +autopart --type=plain +# Partition clearing information +clearpart --linux --initlabel --drives=vda +# Use text mode install +text +repo --name="AppStream" --baseurl=file:///run/install/repo/AppStream +# Use CDROM installation media +cdrom +# Keyboard layouts +keyboard --vckeymap=us --xlayouts='' +# System language +lang en_US.UTF-8 + +# Network information +network --bootproto=dhcp --device=enp0s1 --onboot=off --ipv6=auto --no-activate +network --hostname=localhost.localdomain +# Run the Setup Agent on first boot +firstboot --enable +# Do not configure the X Window System +skipx +# System services +services --enabled="chronyd" +# System timezone +timezone America/New_York --isUtc + +# Shutdown after installation is complete. +shutdown + +%packages +@^server-product-environment +kexec-tools + +%end + +%addon com_redhat_kdump --enable --reserve-mb='auto' + +%end +%anaconda +pwpolicy root --minlen=6 --minquality=1 --notstrict --nochanges --notempty +pwpolicy user --minlen=6 --minquality=1 --notstrict --nochanges --emptyok +pwpolicy luks --minlen=6 --minquality=1 --notstrict --nochanges --notempty +%end diff --git a/tests/vm/centos.aarch64 b/tests/vm/centos.aarch64 new file mode 100755 index 00..73bb9c4f86 --- /dev/null +++ b/tests/vm/centos.aarch64 @@ -0,0 +1,221 @@ +#!/usr/bin/env python +# +# Centos aarch64 image +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# Originally based on ubuntu.aarch64 +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# + +import os +import sys +import subprocess +import basevm +import time +import traceback +import aarch64vm + +DEFAULT_CONFIG = { +'cpu' : "max", +'machine' : "virt,gic-version=max", +'install_cmds' : "yum install -y docker make git python3 gcc, "\ + "yum install -y glib2-devel pixman-devel zlib-devel, "\ + "yum install -y perl-Test-Harness, "\ + "systemctl enable docker", +# We increase beyond the default time since during boot +# it can take some time (many seconds) to log into the VM. +'ssh_timeout' : 60, +} + +class CentosAarch64VM(basevm.BaseVM): +name = "centos.aarch64" +arch = "aarch64" +login_prompt = "localhost login:" +prompt = '[root@localhost ~]#' +image_name = "CentOS-8-aarch64-1905-dvd1.iso" +image_link = "http://mirrors.usc.edu/pub/linux/distributions/centos/8.0.1905/isos/aarch64/; +image_link += image_name +BUILD_SCRIPT = """ +set -e; +cd $(mktemp -d); +sudo chmod a+r /dev/vdb; +tar --checkpoint=.10 -xf /dev/vdb; +./configure {configure_opts}; +make --output-sync {target} -j{jobs} {verbose}; +""" +
[PATCH v1 12/14] tests/vm: Added a new script for ubuntu.aarch64.
ubuntu.aarch64 provides a script to create an Ubuntu 18.04 VM. Another new file is also added aarch64vm.py, which is a module with common methods used by aarch64 VMs, such as how to create the flash images. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 3 +- tests/vm/aarch64vm.py | 100 + tests/vm/basevm.py| 8 +++ tests/vm/ubuntu.aarch64 | 113 ++ 4 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 tests/vm/aarch64vm.py create mode 100755 tests/vm/ubuntu.aarch64 diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index f67438c552..ccafe966cd 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -2,7 +2,7 @@ .PHONY: vm-build-all vm-clean-all -IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 IMAGES_DIR := $(HOME)/.cache/qemu-vm/images IMAGE_FILES := $(patsubst %, $(IMAGES_DIR)/%.img, $(IMAGES)) @@ -18,6 +18,7 @@ vm-help vm-test: @echo " vm-build-openbsd- Build QEMU in OpenBSD VM" @echo " vm-build-centos - Build QEMU in CentOS VM, with Docker" @echo " vm-build-fedora - Build QEMU in Fedora VM" + @echo " vm-build-ubuntu.aarch64 - Build QEMU in ubuntu aarch64 VM" @echo "" @echo " vm-build-all- Build QEMU in all VMs" @echo " vm-clean-all- Clean up VM images" diff --git a/tests/vm/aarch64vm.py b/tests/vm/aarch64vm.py new file mode 100644 index 00..a4c9fea925 --- /dev/null +++ b/tests/vm/aarch64vm.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python +# +# VM testing aarch64 library +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# +import os +import sys +import subprocess +import basevm +from qemu.accel import kvm_available + +# This is the config needed for current version of QEMU. +# This works for both kvm and tcg. +CURRENT_CONFIG = { +'cpu' : "max", +'machine' : "virt,gic-version=max", +} + +# The minimum minor version of QEMU we will support with aarch64 VMs is 3. +# QEMU versions less than 3 have various issues running these VMs. +QEMU_AARCH64_MIN_VERSION = 3 + +# The DEFAULT_CONFIG will default to a version of +# parameters that works for backwards compatibility. +DEFAULT_CONFIG = {'kvm' : {'cpu' : "host", + 'machine' : "virt,gic-version=host"}, + 'tcg' : {'cpu' : "cortex-a57", + 'machine' : "virt"}, +} + +def get_config_defaults(vmcls, default_config): +"""Fetch the configuration defaults for this VM, + taking into consideration the defaults for + aarch64 first, followed by the defaults for this VM.""" +config = default_config +config.update(aarch_get_config_defaults(vmcls)) +return config + +def aarch_get_config_defaults(vmcls): +# Set the defaults for current version of QEMU. +config = CURRENT_CONFIG +args, argv = basevm.parse_args(vmcls) +qemu_path = basevm.get_qemu_path(vmcls.arch, args.build_path) +qemu_version = basevm.get_qemu_version(qemu_path) +if qemu_version < QEMU_AARCH64_MIN_VERSION: +error = "\nThis major version of QEMU {} is to old for aarch64 VMs.\n"\ +"The major version must be at least {}.\n"\ +"To continue with the current build of QEMU, "\ +"please restart with QEMU_LOCAL=1 .\n" +print(error.format(qemu_version, QEMU_AARCH64_MIN_VERSION)) +exit(1) +if qemu_version == QEMU_AARCH64_MIN_VERSION: +# We have an older version of QEMU, +# set the config values for backwards compatibility. +if kvm_available('aarch64'): +config.update(DEFAULT_CONFIG['kvm']) +else: +config.update(DEFAULT_CONFIG['tcg']) +return config + +def create_flash_images(flash_dir="./"): +"""Creates the appropriate pflash files + for an aarch64 VM.""" +flash0_path = get_flash_path(flash_dir, "flash0") +flash1_path = get_flash_path(flash_dir, "flash1") +subprocess.check_call(["dd", "if=/dev/zero", "of={}".format(flash0_path), + "bs=1M", "count=64"]) +# A reliable way to get the QEMU EFI image is via an installed package. +efi_img = "/usr/share/qemu-efi-aarch64/QEMU_EFI.fd" +if not os.path.exists(e
[PATCH v1 10/14] tests/vm: Add ability to select QEMU from current build.
Added a new special variable QEMU_LOCAL=1, which will indicate to take the QEMU binary from the current build. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 4 tests/vm/basevm.py| 28 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index d72babd5bf..f67438c552 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -34,6 +34,7 @@ vm-help vm-test: @echo "DEBUG=1 - Enable verbose output on host and interactive debugging" @echo "BOOT_CONSOLE=1- Show the console output at boot time. " @echo "V=1 - Enable verbose ouput on host and guest commands" + @echo "QEMU_LOCAL=1 - Use QEMU binary local to this build." @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool" @echo "QEMU_CONFIG=/path/conf.yml - Change path to VM configuration .yml file." @@ -52,6 +53,7 @@ $(IMAGES_DIR)/%.img: $(SRC_PATH)/tests/vm/% \ $(PYTHON) $< \ $(if $(V)$(DEBUG), --debug) \ $(if $(BOOT_CONSOLE),--boot-console) \ + --build-path $(BUILD_DIR)\ --image "$@" \ --force \ --build-image $@, \ @@ -67,6 +69,7 @@ vm-build-%: $(IMAGES_DIR)/%.img $(if $(J),--jobs $(J)) \ $(if $(V),--verbose) \ $(if $(BOOT_CONSOLE),--boot-console) \ + --build-path $(BUILD_DIR)\ --image "$<" \ $(if $(BUILD_TARGET),--build-target $(BUILD_TARGET)) \ --snapshot \ @@ -88,6 +91,7 @@ vm-boot-ssh-%: $(IMAGES_DIR)/%.img $(PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(J),--jobs $(J)) \ $(if $(BOOT_CONSOLE),--boot-console) \ + --build-path $(BUILD_DIR)\ --image "$<" \ --interactive \ false, \ diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index b99ab0f20a..97d55f8030 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -97,9 +97,10 @@ class BaseVM(object): tcg_ssh_timeout_multiplier = 4 console_logfile = "console.log" def __init__(self, debug=False, vcpus=None, config=None, -boot_console=None): +boot_console=None, build_path=None): self._guest = None self._console_fd = None +self._build_path = build_path self._boot_console = boot_console self._socket_thread = None self._console_timeout_sec = self.socket_timeout @@ -287,8 +288,8 @@ class BaseVM(object): args += self._data_args + extra_args + self._config['extra_args'] args += ["-device", "VGA"] logging.debug("QEMU args: %s", " ".join(args)) -qemu_bin = os.environ.get("QEMU", "qemu-system-" + self.arch) -guest = QEMUMachine(binary=qemu_bin, args=args) +qemu_path = get_qemu_path(self.arch, self._build_path) +guest = QEMUMachine(binary=qemu_path, args=args) guest.set_machine(self._config['machine']) guest.set_console() try: @@ -505,6 +506,22 @@ class BaseVM(object): stderr=self._stdout) return os.path.join(cidir, "cloud-init.iso") +def get_qemu_path(arch, build_path=None): +"""Fetch the path to the qemu binary.""" +qemu_local = os.environ.get("QEMU_LOCAL", 0) +# If QEMU environment variable set, it takes precedence +if "QEMU" in os.environ: +qemu_path = os.environ["QEMU"] +elif qemu_local: +if not build_path: +raise Exception("--build-path option required with QEMU_LOCAL") +qemu_path = os.path.join(build_path, arch + "-softmmu") +qemu_path = os.path.join(qemu_path, "qemu-system-" + arch) +else: +# Default is to use system path for qemu. +qemu_path = "qemu-system-" + arch +return qemu_path + def parse_config(config, args): """ Parse yaml config and populate our config structure. The yaml config allows the user to override the @@ -567,6 +584,8 @@ def parse_args(vmcls): "See config_example.yaml for example.") parser.add_option("--boot-console", action="store_true", help="Show console during boot. ") +parser.add_option("--build-path", default=None, +
Re: [PATCH v3 02/17] tests/docker: better handle symlinked libs
On Mon, 3 Feb 2020 at 04:09, Alex Bennée wrote: > Subject: [PATCH v3 02/17] tests/docker: better handle symlinked libs > > When we are copying we want to ensure we grab the first > resolution (the found in path section). However even that binary might > be a symlink so lets make sure we chase the symlinks to copy the right > binary to where it can be found. > > Signed-off-by: Alex Bennée > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé Reviewed-by: Robert Foley
Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode
Hi, On Fri, 7 Feb 2020 at 10:01, Alex Bennée wrote: > -static void decode_RV32_64C0(DisasContext *ctx) > +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode) > { > -uint8_t funct3 = extract32(ctx->opcode, 13, 3); > -uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode); > -uint8_t rs1s = GET_C_RS1S(ctx->opcode); > +uint8_t funct3 = extract32(opcode, 13, 3); We noticed that a uint16_t opcode is passed into this function and then passed on to extract32(). This is a minor point, but the extract32() will validate the start and length values passed in according to 32 bits, not 16 bits. static inline uint32_t extract32(uint32_t value, int start, int length) { assert(start >= 0 && length > 0 && length <= 32 - start); Since we have an extract32() and extract64(), maybe we could add an extract16() and use it here? Thanks & Regards, -Rob > +uint8_t rd_rs2 = GET_C_RS2S(opcode); > +uint8_t rs1s = GET_C_RS1S(opcode); > > switch (funct3) { > case 3: > #if defined(TARGET_RISCV64) > /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/ > gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s, > - GET_C_LD_IMM(ctx->opcode)); > + GET_C_LD_IMM(opcode)); > #else > /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/ > gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s, > -GET_C_LW_IMM(ctx->opcode)); > +GET_C_LW_IMM(opcode)); > #endif > break; > case 7: > #if defined(TARGET_RISCV64) > /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/ > gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2, > - GET_C_LD_IMM(ctx->opcode)); > + GET_C_LD_IMM(opcode)); > #else > /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/ > gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2, > - GET_C_LW_IMM(ctx->opcode)); > + GET_C_LW_IMM(opcode)); > #endif > break; > } > } > > -static void decode_RV32_64C(DisasContext *ctx) > +static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode) > { > -uint8_t op = extract32(ctx->opcode, 0, 2); > +uint8_t op = extract32(opcode, 0, 2); > > switch (op) { > case 0: > -decode_RV32_64C0(ctx); > +decode_RV32_64C0(ctx, opcode); > break; > } > } > @@ -709,22 +708,24 @@ static bool gen_shift(DisasContext *ctx, arg_r *a, > /* Include the auto-generated decoder for 16 bit insn */ > #include "decode_insn16.inc.c" > > -static void decode_opc(DisasContext *ctx) > +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t > opcode) > { > /* check for compressed insn */ > -if (extract32(ctx->opcode, 0, 2) != 3) { > +if (extract32(opcode, 0, 2) != 3) { > if (!has_ext(ctx, RVC)) { > gen_exception_illegal(ctx); > } else { > ctx->pc_succ_insn = ctx->base.pc_next + 2; > -if (!decode_insn16(ctx, ctx->opcode)) { > +if (!decode_insn16(ctx, opcode)) { > /* fall back to old decoder */ > -decode_RV32_64C(ctx); > +decode_RV32_64C(ctx, opcode); > } > } > } else { > +uint32_t opcode32 = opcode; > +opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, > ctx->base.pc_next + 2)); > ctx->pc_succ_insn = ctx->base.pc_next + 4; > -if (!decode_insn32(ctx, ctx->opcode)) { > +if (!decode_insn32(ctx, opcode32)) { > gen_exception_illegal(ctx); > } > } > @@ -776,9 +777,9 @@ static void riscv_tr_translate_insn(DisasContextBase > *dcbase, CPUState *cpu) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > CPURISCVState *env = cpu->env_ptr; > +uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next); > > -ctx->opcode = translator_ldl(env, ctx->base.pc_next); > -decode_opc(ctx); > +decode_opc(env, ctx, opcode16); > ctx->base.pc_next = ctx->pc_succ_insn; > > if (ctx->base.is_jmp == DISAS_NEXT) { > -- > 2.20.1 >
Re: [PATCH v1 1/5] docs/devel: document query handle lifetimes
On Fri, 7 Feb 2020 at 10:01, Alex Bennée wrote: > > I forgot to document the lifetime of handles in the developer > documentation. Do so now. > > Signed-off-by: Alex Bennée Reviewed-by: Robert Foley Regards, -Rob > --- > docs/devel/tcg-plugins.rst | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst > index 718eef00f22..a05990906cc 100644 > --- a/docs/devel/tcg-plugins.rst > +++ b/docs/devel/tcg-plugins.rst > @@ -51,8 +51,17 @@ about how QEMU's translation works to the plugins. While > there are > conceptions such as translation time and translation blocks the > details are opaque to plugins. The plugin is able to query select > details of instructions and system configuration only through the > -exported *qemu_plugin* functions. The types used to describe > -instructions and events are opaque to the plugins themselves. > +exported *qemu_plugin* functions. > + > +Query Handle Lifetime > +- > + > +Each callback provides an opaque anonymous information handle which > +can usually be further queried to find out information about a > +translation, instruction or operation. The handles themselves are only > +valid during the lifetime of the callback so it is important that any > +information that is needed is extracted during the callback and saved > +by the plugin. > > Usage > = > -- > 2.20.1 >
Re: [PATCH v1 06/14] tests/vm: Add logging of console to file.
On Fri, 7 Feb 2020 at 12:12, Alex Bennée wrote: > Robert Foley writes: > > > This adds logging of the char device used by the console > > to a file. The basevm.py then uses this file to read > > chars from the console. > > One reason to add this is to aid with debugging. > > I can certainly see an argument for saving the install log. > > > But another is because there is an issue where the QEMU > > might hang if the characters from the character device > > are not consumed by the script. > > I'm a little confused by this. Outputting to a file and then parsing the > file seems a bit more janky than injesting the output in the script and > then logging it. > > Is this to work around the hang because the socket buffers fill up and > aren't drained? Yes, exactly. This is to work around the hang we are seeing when we try to use these new VMs. > > +console_logfile = "console.log" > > We should probably dump the log somewhere other than cwd. Given we cache > stuff in ~/.cache/qemu-vm maybe something of the form: > > ~/.cache/qemu-vm/ubuntu.aarch64.install.log > > ? Good point, we will locate the log file there. > > +elapsed_sec = time.time() - start_time > > +if elapsed_sec > self._console_timeout_sec: > > +raise ConsoleTimeoutException > > +return data.encode('latin1') > > + > > Is latin1 really the best choice here? I would expect things to be utf-8 > clean. We were trying to follow the existing code, which is using latin1. For example, console_wait() or console_consume() are using latin1. However on further inspection we see that console_send() is using utf-8. We will look at changing these latin1 cases to be utf-8. I also found a case in get_qemu_version() we will change to utf-8 also. > > + > > +def join(self, timeout=None): > > +"""Time to destroy the thread. > > + Clear the event to stop the thread, and wait for > > + it to complete.""" > > +self.alive.clear() > > +threading.Thread.join(self, timeout) > > +self.log_file.close() > > I'm note sure about this - introducing threading into Python seems very > un-pythonic. I wonder if the python experts have any view on a better > way to achieve what we want which I think is: > > - a buffer that properly drains output from QEMU > - which can optionally be serialised onto disk for logging > > What else are we trying to achieve here? I think that covers what we are trying to achieve here. The logging to file is a nice to have, but the draining of the output from QEMU is the main objective here. We will do a bit more research here to seek out a cleaner way to achieve this, but certainly we would also be interested if any python experts have a view on a cleaner approach. Thanks & Regards, -Rob > > -- > Alex Bennée
Re: [PATCH 2/8] tests/vm: increased max timeout for vm boot.
Hi Phil, > I once suggested "When using TCG, wait longer for a VM to start" > https://www.mail-archive.com/qemu-devel@nongnu.org/msg550610.html > Thanks for the pointer. This increase in timeout under TCG seems just right for the case we saw. I will incorporate this into the patch. Thanks & Regards, -Rob On Fri, 24 Jan 2020 at 12:12, Philippe Mathieu-Daudé wrote: > > Hi Robert, > > On 1/24/20 5:53 PM, Robert Foley wrote: > > Add change to increase timeout waiting for VM to boot. > > Needed for some emulation cases where it can take longer > > than 5 minutes to boot. > > > > Signed-off-by: Robert Foley > > Reviewed-by: Peter Puhov > > --- > > tests/vm/basevm.py | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py > > index 991115e44b..86908f58ec 100755 > > --- a/tests/vm/basevm.py > > +++ b/tests/vm/basevm.py > > @@ -310,7 +310,7 @@ class BaseVM(object): > > def print_step(self, text): > > sys.stderr.write("### %s ...\n" % text) > > > > -def wait_ssh(self, seconds=300): > > +def wait_ssh(self, seconds=600): > > starttime = datetime.datetime.now() > > endtime = starttime + datetime.timedelta(seconds=seconds) > > guest_up = False > > > > I once suggested "When using TCG, wait longer for a VM to start" > https://www.mail-archive.com/qemu-devel@nongnu.org/msg550610.html > > Cleber took some notes about 'kicking a expiring timer' but I can't find > it. This might be related: > https://trello.com/c/MYdgH4mz/90-delayed-failures > > Regards, > > Phil. >
[PATCH 2/8] tests/vm: increased max timeout for vm boot.
Add change to increase timeout waiting for VM to boot. Needed for some emulation cases where it can take longer than 5 minutes to boot. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 991115e44b..86908f58ec 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -310,7 +310,7 @@ class BaseVM(object): def print_step(self, text): sys.stderr.write("### %s ...\n" % text) -def wait_ssh(self, seconds=300): +def wait_ssh(self, seconds=600): starttime = datetime.datetime.now() endtime = starttime + datetime.timedelta(seconds=seconds) guest_up = False -- 2.17.1
[PATCH 0/8] tests/vm: Add support for aarch64 VMs
This patch adds support for 2 aarch64 VMs. - Ubuntu 18.04 aarch64 VM - CentOS 8 aarch64 VM In order to add support for the two new aarch64 VMs, we generalized and parameterized basevm.py. We added a new concept of a configuration, which is really just a set of parameters which define how to configure the VM. Some examples of parameters are "machine", "memory" and "cpu". We preserved current default parameters. Current configuration of pre-existing VMs is supported by default without need to override default parameters. For example, previously only the 'pc' machine was supported in basevm.py. The new aarch64 VMs will override machine to use virt. There are a few other examples where we needed to add parameters in order to add support for these aarch64 VMs. In some other cases we added parameters that we thought would be useful in general, for example username/password, ssh keys, In the case of the aarch64 VMs, they override certain parameters by default. However, it is also of value to be able to dynamically specify other values for these parameters. Take the case where you create a new VM using vm-build, but then want to test it using various hardware configurations such as for example different NUMA topologies. Or maybe you want to use a different amount of memory or a different cpu type. In order to support these use cases we added support for a configuration .yml file, which allows the user to specify certain values dynamically such as: - machine - cpu - memory size - other qemu args, which allow configuring alternate hardware topologies such as NUMA nodes. - username, password - ssh keys For an example of a .yml file, see the included config_example.yml The main use case for using this config.yml file is for where we are testing/debugging with qemu (vm-build), and need to configure the VM differently. However, there is another use case we have developed, which is a project called lisa-qemu (https://github.com/rf972/lisa-qemu). This project is an integration between the LISA tool and QEMU. This project uses the VMs created by QEMU's vm-build scripts for use in testing with LISA. This use case is similar to the vm-build case in that, the VM gets created once, and we want to launch the VM with different configurations (memory, cpu, etc.). As part of developing the scripts for these VMs, we implemented a few enhancements to help with testing. For example, we added support for allowing debug mode to show the ssh output. We also added support for a new --boot-console option which will show the boot console as the VM boots up to aid in debugging problems during VM boot. Robert Foley (8): tests/vm: Debug mode shows ssh output. tests/vm: increased max timeout for vm boot. tests/vm: change wait_ssh to optionally wait for root. tests/vm: Add configuration to basevm.py tests/vm: Added configuration file support tests/vm: add --boot-console switch tests/vm: Added a new script for ubuntu.aarch64. tests/vm: Added a new script for centos.aarch64. tests/vm/Makefile.include| 8 +- tests/vm/aarch64vm.py| 41 +++ tests/vm/basevm.py | 192 +- tests/vm/centos-8-aarch64.ks | 52 + tests/vm/centos.aarch64 | 218 +++ tests/vm/config_example.yml | 52 + tests/vm/ubuntu.aarch64 | 144 +++ 7 files changed, 675 insertions(+), 32 deletions(-) create mode 100644 tests/vm/aarch64vm.py create mode 100644 tests/vm/centos-8-aarch64.ks create mode 100755 tests/vm/centos.aarch64 create mode 100644 tests/vm/config_example.yml create mode 100755 tests/vm/ubuntu.aarch64 -- 2.17.1
[PATCH 1/8] tests/vm: Debug mode shows ssh output.
Add changes to tests/vm/basevm.py so that during debug mode we show ssh output. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index ed5dd4f3d0..991115e44b 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -122,11 +122,16 @@ class BaseVM(object): return fname def _ssh_do(self, user, cmd, check): -ssh_cmd = ["ssh", "-q", "-t", +ssh_cmd = ["ssh", + "-t", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=" + os.devnull, "-o", "ConnectTimeout=1", "-p", self.ssh_port, "-i", self._ssh_key_file] +# If not in debug mode, set ssh to quiet mode to +# avoid printing the results of commands. +if not self.debug: +ssh_cmd.append("-q") for var in self.envvars: ssh_cmd += ['-o', "SendEnv=%s" % var ] assert not isinstance(cmd, str) -- 2.17.1
[PATCH 6/8] tests/vm: add --boot-console switch
Added ability to view console during boot via --boot-console switch. This helps debug issues that occur during the boot sequence. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 08a8989ac0..aa8b39beb7 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -489,6 +489,8 @@ def parse_args(vmcls): parser.add_option("--config", "-c", default=None, help="Provide config yaml for configuration. "\ "See config_example.yaml for example.") +parser.add_option("--boot-console", action="store_true", + help="Show console during boot. ") parser.disable_interspersed_args() return parser.parse_args() @@ -523,6 +525,10 @@ def main(vmcls, config=None): if args.snapshot: img += ",snapshot=on" vm.boot(img) +wait_boot = getattr(vm, "wait_boot", None) +if args.boot_console and callable(wait_boot): +vm.console_init() +wait_boot() vm.wait_ssh() except Exception as e: if isinstance(e, SystemExit) and e.code == 0: -- 2.17.1
[PATCH 8/8] tests/vm: Added a new script for centos.aarch64.
centos.aarch64 creates a CentOS 8 image. Also added a new kickstart script used to build the centos.aarch64 image. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/Makefile.include| 3 +- tests/vm/centos-8-aarch64.ks | 52 + tests/vm/centos.aarch64 | 218 +++ 3 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 tests/vm/centos-8-aarch64.ks create mode 100755 tests/vm/centos.aarch64 diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 966b417ba7..febf82fe16 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -2,7 +2,7 @@ .PHONY: vm-build-all vm-clean-all -IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 centos.aarch64 IMAGES_DIR := $(HOME)/.cache/qemu-vm/images IMAGE_FILES := $(patsubst %, $(IMAGES_DIR)/%.img, $(IMAGES)) @@ -19,6 +19,7 @@ vm-help vm-test: @echo " vm-build-centos - Build QEMU in CentOS VM, with Docker" @echo " vm-build-fedora - Build QEMU in Fedora VM" @echo " vm-build-ubuntu.aarch64 - Build QEMU in ubuntu aarch64 VM" + @echo " vm-build-centos.aarch64 - Build QEMU in CentOS aarch64 VM" @echo "" @echo " vm-build-all- Build QEMU in all VMs" @echo " vm-clean-all- Clean up VM images" diff --git a/tests/vm/centos-8-aarch64.ks b/tests/vm/centos-8-aarch64.ks new file mode 100644 index 00..5f6093fefa --- /dev/null +++ b/tests/vm/centos-8-aarch64.ks @@ -0,0 +1,52 @@ +# CentOS aarch64 image kickstart file. +# This file is used by the CentOS installer to +# script the generation of the image. +# +# Copyright 2020 Linaro +# +ignoredisk --only-use=vda +# System bootloader configuration +bootloader --append=" crashkernel=auto" --location=mbr --boot-drive=vda +autopart --type=plain +# Partition clearing information +clearpart --linux --initlabel --drives=vda +# Use text mode install +text +repo --name="AppStream" --baseurl=file:///run/install/repo/AppStream +# Use CDROM installation media +cdrom +# Keyboard layouts +keyboard --vckeymap=us --xlayouts='' +# System language +lang en_US.UTF-8 + +# Network information +network --bootproto=dhcp --device=enp0s1 --onboot=off --ipv6=auto --no-activate +network --hostname=localhost.localdomain +# Run the Setup Agent on first boot +firstboot --enable +# Do not configure the X Window System +skipx +# System services +services --enabled="chronyd" +# System timezone +timezone America/New_York --isUtc + +# Shutdown after installation is complete. +shutdown + +%packages +@^server-product-environment +kexec-tools + +%end + +%addon com_redhat_kdump --enable --reserve-mb='auto' + +%end +%anaconda +pwpolicy root --minlen=6 --minquality=1 --notstrict --nochanges --notempty +pwpolicy user --minlen=6 --minquality=1 --notstrict --nochanges --emptyok +pwpolicy luks --minlen=6 --minquality=1 --notstrict --nochanges --notempty +%end + diff --git a/tests/vm/centos.aarch64 b/tests/vm/centos.aarch64 new file mode 100755 index 00..d939c2a900 --- /dev/null +++ b/tests/vm/centos.aarch64 @@ -0,0 +1,218 @@ +#!/usr/bin/env python +# +# Centos aarch64 image +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# Originally based on ubuntu.aarch64 +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# + +import os +import sys +import subprocess +import basevm +import time +import traceback +import aarch64vm + +DEFAULT_CONFIG = { +'cpu' : "max", +'machine' : "virt,gic-version=max", +'install_cmds' : "yum install -y docker make git python3 gcc, "\ + "yum install -y glib2-devel pixman-devel zlib-devel, "\ + "yum install -y perl-Test-Harness, "\ + "systemctl enable docker", +# We increase beyond the default time since during boot +# it can take some time (many seconds) to log into the VM. +'ssh_timeout' : 60, +} + +class CentosAarch64VM(basevm.BaseVM): +name = "centos.aarch64" +arch = "aarch64" +login_prompt = "localhost login:" +prompt = '[root@localhost ~]#' +image_name = "CentOS-8-aarch64-1905-dvd1.iso" +image_link = "http://mirrors.usc.edu/pub/linux/distributions/centos/8.0.1905/isos/aarch64/; +image_link += image_name +BUILD_SCRIPT = """ +set -e; +cd $(mktemp -d); +sudo chmod a+r /dev/vdb; +tar --checkpoint=.10 -xf /dev/vdb; +./configure {configure_opts}; +make --output-sync {target} -j{jobs} {verbose}; +""&
[PATCH 5/8] tests/vm: Added configuration file support
Changes to tests/vm/basevm.py to allow accepting a configuration file as a parameter. Allows for specifying VM options such as cpu, machine, memory, and arbitrary qemu arguments for specifying options such as NUMA configuration. Also added an example config_example.yml. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 60 + tests/vm/config_example.yml | 52 2 files changed, 112 insertions(+) create mode 100644 tests/vm/config_example.yml diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index ec92c8f105..08a8989ac0 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -31,6 +31,7 @@ import tempfile import shutil import multiprocessing import traceback +import yaml SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), "..", "keys", "id_rsa") @@ -396,6 +397,61 @@ class BaseVM(object): def qmp(self, *args, **kwargs): return self._guest.qmp(*args, **kwargs) + +def parse_config(config, args): +""" Parse yaml config and populate our config structure. +The yaml config allows the user to override the +defaults for VM parameters. In many cases these +defaults can be overridden without rebuilding the VM.""" +if args.config: +config_file = args.config +elif 'QEMU_CONFIG' in os.environ: +config_file = os.environ['QEMU_CONFIG'] +else: +return config +if not os.path.exists(config_file): +raise Exception("config file {} does not exist".format(config_file)) +with open(config_file) as f: +yaml_dict = yaml.safe_load(f) +if 'target-conf' in yaml_dict: +target_dict = yaml_dict['target-conf'] +if 'username' in target_dict and target_dict['username'] != 'root': +config['guest_user'] = target_dict['username'] +if 'password' in target_dict: +config['root_pass'] = target_dict['password'] +config['guest_pass'] = target_dict['password'] +if any (k in target_dict for k in ("ssh_key","ssh_pub_key")) and \ + not all (k in target_dict for k in ("ssh_key","ssh_pub_key")): +missing_key = "ssh_pub_key" \ + if 'ssh_key' in target_dict else "ssh_key" +raise Exception("both ssh_key and ssh_pub_key required. " +"{} key is missing.".format(missing_key)) +if 'ssh_key' in target_dict: +config['ssh_key_file'] = target_dict['ssh_key'] +if not os.path.exists(config['ssh_key_file']): +raise Exception("ssh key file not found.") +if 'ssh_pub_key' in target_dict: +config['ssh_pub_key_file'] = target_dict['ssh_pub_key'] +if not os.path.exists(config['ssh_pub_key_file']): +raise Exception("ssh pub key file not found.") +if 'machine' in target_dict: +config['machine'] = target_dict['machine'] +if 'qemu_args' in target_dict: +qemu_args = target_dict['qemu_args'] +qemu_args = qemu_args.replace('\n', ' ').replace('\r', '') +config['extra_args'] = qemu_args.split(' ') +if 'memory' in target_dict: +config['memory'] = target_dict['memory'] +if 'dns' in target_dict: +config['dns'] = target_dict['dns'] +if 'cpu' in target_dict: +config['cpu'] = target_dict['cpu'] +if 'ssh_port' in target_dict: +config['ssh_port'] = target_dict['ssh_port'] +if 'install_cmds' in target_dict: +config['install_cmds'] = target_dict['install_cmds'] +return config + def parse_args(vmcls): def get_default_jobs(): @@ -430,6 +486,9 @@ def parse_args(vmcls): help="Interactively run command") parser.add_option("--snapshot", "-s", action="store_true", help="run tests with a snapshot") +parser.add_option("--config", "-c", default=None, + help="Provide config yaml for configuration. "\ + "See config_example.yaml for example.") parser.disable_interspersed_args() return parser.parse_args() @@ -441,6 +500,7 @@ def main(vmcls, config=None): if not argv and not args.build_qemu and not args.build_image: print("Nothing to do?") return 1 +config = parse_config(config, args) logging.basicConfig(level=(logging.DEBUG if args.debug else logging.WARN)) vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config) diff --git a/tests/vm/config_example.yml b/tests/vm/config_example.y
[PATCH 7/8] tests/vm: Added a new script for ubuntu.aarch64.
ubuntu.aarch64 provides a script to create an Ubuntu 18.04 VM. Another new file is also added aarch64vm.py, which is a module with common methods used by aarch64 VMs, such as how to create the flash images. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/Makefile.include | 7 +- tests/vm/aarch64vm.py | 41 +++ tests/vm/ubuntu.aarch64 | 144 ++ 3 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 tests/vm/aarch64vm.py create mode 100755 tests/vm/ubuntu.aarch64 diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 9e7c46a473..966b417ba7 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -2,7 +2,7 @@ .PHONY: vm-build-all vm-clean-all -IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 IMAGES_DIR := $(HOME)/.cache/qemu-vm/images IMAGE_FILES := $(patsubst %, $(IMAGES_DIR)/%.img, $(IMAGES)) @@ -18,6 +18,7 @@ vm-help vm-test: @echo " vm-build-openbsd- Build QEMU in OpenBSD VM" @echo " vm-build-centos - Build QEMU in CentOS VM, with Docker" @echo " vm-build-fedora - Build QEMU in Fedora VM" + @echo " vm-build-ubuntu.aarch64 - Build QEMU in ubuntu aarch64 VM" @echo "" @echo " vm-build-all- Build QEMU in all VMs" @echo " vm-clean-all- Clean up VM images" @@ -35,6 +36,8 @@ vm-help vm-test: @echo "V=1 - Enable verbose ouput on host and guest commands" @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool" + @echo "QEMU_CONFIG=/path/conf.yml- Change path to VM configuration .yml file." + @echo "See config_example.yml for file format details." vm-build-all: $(addprefix vm-build-, $(IMAGES)) @@ -80,7 +83,7 @@ vm-boot-serial-%: $(IMAGES_DIR)/%.img vm-boot-ssh-%: $(IMAGES_DIR)/%.img $(call quiet-command, \ - $(SRC_PATH)/tests/vm/$* \ + $(PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(J),--jobs $(J)) \ --image "$<" \ --interactive \ diff --git a/tests/vm/aarch64vm.py b/tests/vm/aarch64vm.py new file mode 100644 index 00..43f841571f --- /dev/null +++ b/tests/vm/aarch64vm.py @@ -0,0 +1,41 @@ +#!/usr/bin/env python +# +# VM testing aarch64 library +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# +import os +import sys +import subprocess +import basevm + + +def create_flash_images(): +"""Creates the appropriate pflash files + for an aarch64 VM.""" +subprocess.check_call(["dd", "if=/dev/zero", "of=flash0.img", + "bs=1M", "count=64"]) +# A reliable way to get the QEMU EFI image is via an installed package. +efi_img = "/usr/share/qemu-efi-aarch64/QEMU_EFI.fd" +if not os.path.exists(efi_img): +sys.stderr.write("*** {} is missing\n".format(efi_img)) +sys.stderr.write("*** please install qemu-efi-aarch64 package\n") +exit(3) +subprocess.check_call(["dd", "if={}".format(efi_img), + "of=flash0.img", "conv=notrunc"]) +subprocess.check_call(["dd", "if=/dev/zero", + "of=flash1.img", "bs=1M", "count=64"]) + +def get_pflash_args(): +"""Returns a string that can be used to + boot qemu using the appropriate pflash files + for aarch64.""" +pflash_args = "-drive file=flash0.img,format=raw,if=pflash "\ + "-drive file=flash1.img,format=raw,if=pflash" +return pflash_args.split(" ") diff --git a/tests/vm/ubuntu.aarch64 b/tests/vm/ubuntu.aarch64 new file mode 100755 index 00..941f7f5166 --- /dev/null +++ b/tests/vm/ubuntu.aarch64 @@ -0,0 +1,144 @@ +#!/usr/bin/env python +# +# Ubuntu aarch64 image +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# Originally based on ubuntu.i386 Fam Zheng +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# + +import os +import sys +import subprocess +import basevm +import time +import aarch64vm + +DEFAULT_CONFIG = { +'cpu' : "max", +'machin
[PATCH 4/8] tests/vm: Add configuration to basevm.py
Added use of a configuration to tests/vm/basevm.py. The configuration provides parameters used to configure a VM. This allows for providing alternate configurations to the VM being created/launched. cpu, machine, memory, and NUMA configuration are all examples of configuration which we might want to vary on the VM being created or launched. This will for example allow for creating an aarch64 vm. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 108 ++--- 1 file changed, 82 insertions(+), 26 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 3b4403ddcb..ec92c8f105 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -32,15 +32,40 @@ import shutil import multiprocessing import traceback -SSH_KEY = open(os.path.join(os.path.dirname(__file__), - "..", "keys", "id_rsa")).read() -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), - "..", "keys", "id_rsa.pub")).read() - +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), + "..", "keys", "id_rsa") +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), + "..", "keys", "id_rsa.pub") +SSH_KEY = open(SSH_KEY_FILE).read() +SSH_PUB_KEY = open(SSH_PUB_KEY_FILE).read() + +# This is the standard configuration. +# Any or all of these can be overridden by +# passing in a config argument to the VM constructor. +DEFAULT_CONFIG = { +'cpu' : "max", +'machine' : 'pc', +'guest_user' : "qemu", +'guest_pass' : "qemupass", +'root_pass' : "qemupass", +'ssh_key_file': SSH_KEY_FILE, +'ssh_pub_key_file': SSH_PUB_KEY_FILE, +'memory' : "4G", +'extra_args' : [], +'dns' : "", +'ssh_port': 0, +'install_cmds': "", +'boot_dev_type' : "block", +'ssh_timeout' : 1, +} +BOOT_DEVICE = { +'block' : "-drive file={},if=none,id=drive0,cache=writeback "\ + "-device virtio-blk,drive=drive0,bootindex=0", +'scsi' : "-device virtio-scsi-device,id=scsi "\ + "-drive file={},format=raw,if=none,id=hd0 "\ + "-device scsi-hd,drive=hd0,bootindex=0", +} class BaseVM(object): -GUEST_USER = "qemu" -GUEST_PASS = "qemupass" -ROOT_PASS = "qemupass" envvars = [ "https_proxy", @@ -59,19 +84,26 @@ class BaseVM(object): poweroff = "poweroff" # enable IPv6 networking ipv6 = True -def __init__(self, debug=False, vcpus=None): +def __init__(self, debug=False, vcpus=None, config=None): self._guest = None +# Allow input config to override defaults. +self._config = DEFAULT_CONFIG.copy() +if config != None: +self._config.update(config) self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", suffix=".tmp", dir=".")) atexit.register(shutil.rmtree, self._tmpdir) - +self._config['ssh_key'] = \ +open(self._config['ssh_key_file']).read().rstrip() +self._config['ssh_pub_key'] = \ +open(self._config['ssh_pub_key_file']).read().rstrip() self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") -open(self._ssh_key_file, "w").write(SSH_KEY) +open(self._ssh_key_file, "w").write(self._config['ssh_key']) subprocess.check_call(["chmod", "600", self._ssh_key_file]) self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") -open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY) +open(self._ssh_pub_key_file, "w").write(self._config['ssh_pub_key']) self.debug = debug self._stderr = sys.stderr @@ -80,11 +112,14 @@ class BaseVM(object): self._stdout = sys.stdout else: self._stdout = self._devnull +netdev = "user,id=vnet,hostfwd=:127.0.0.1:{}-:22" self._args = [ \ -"-nodefaults", "-m", "4G", -"-cpu", "max", -"-netdev", "user,id=vnet,hostfwd=:127.0.0.1:0-:22" + - (",ipv6=no" if not self.ipv6 else ""), +"-nodefaults", "-m", self._config['memory'], +"-cpu", self._config['cpu'], +"-netdev", +netdev.format(self._config['ssh_
[PATCH 3/8] tests/vm: change wait_ssh to optionally wait for root.
Allow wait_ssh to wait for root user to be ready. This solves the issue where we perform a wait_ssh() successfully, but the root user is not yet ready to be logged in. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 86908f58ec..3b4403ddcb 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -310,12 +310,17 @@ class BaseVM(object): def print_step(self, text): sys.stderr.write("### %s ...\n" % text) -def wait_ssh(self, seconds=600): +def wait_ssh(self, wait_root=False, seconds=600): starttime = datetime.datetime.now() endtime = starttime + datetime.timedelta(seconds=seconds) guest_up = False while datetime.datetime.now() < endtime: -if self.ssh("exit 0") == 0: +if wait_root: +if self.ssh("exit 0") == 0 and\ + self.ssh_root("exit 0") == 0: +guest_up = True +break +elif self.ssh("exit 0") == 0: guest_up = True break seconds = (endtime - datetime.datetime.now()).total_seconds() -- 2.17.1
Re: [PATCH 4/8] tests/vm: Add configuration to basevm.py
On Mon, 27 Jan 2020 at 07:26, Alex Bennée wrote: > > -SSH_KEY = open(os.path.join(os.path.dirname(__file__), > > - "..", "keys", "id_rsa")).read() > > -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), > > - "..", "keys", "id_rsa.pub")).read() > > - > > +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), > > + "..", "keys", "id_rsa") > > +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), > > + "..", "keys", "id_rsa.pub") > > +SSH_KEY = open(SSH_KEY_FILE).read() > > +SSH_PUB_KEY = open(SSH_PUB_KEY_FILE).read() > > Why are we tracking more information about the keyfile than we used to > now? Is this because it's harder to embed keys over paths in the config? We now allow the user to override the ssh keys. Because of this we need to track the filename separately from the contents of the file, so that we can put this default filename into the DEFAULT_CONFIG below. We also keep the original SSH_PUB_KEY since it is still used by some pre-existing VM scripts, and we did not want to break backwards compatibility for those scripts. Actually upon further inspection, it looks like we can delete SSH_KEY, this is no longer needed. :) > > > + > > +# This is the standard configuration. > > +# Any or all of these can be overridden by > > +# passing in a config argument to the VM constructor. > > +DEFAULT_CONFIG = { > > +'cpu' : "max", > > +'machine' : 'pc', > > +'guest_user' : "qemu", > > +'guest_pass' : "qemupass", > > +'root_pass' : "qemupass", > > +'ssh_key_file': SSH_KEY_FILE, > > +'ssh_pub_key_file': SSH_PUB_KEY_FILE, > > +'memory' : "4G", > > +'extra_args' : [], > > +'dns' : "", > > +'ssh_port': 0, > > +'install_cmds': "", > > +'boot_dev_type' : "block", > > +'ssh_timeout' : 1, > > +} > > +BOOT_DEVICE = { > > +'block' : "-drive file={},if=none,id=drive0,cache=writeback "\ > > + "-device virtio-blk,drive=drive0,bootindex=0", > > +'scsi' : "-device virtio-scsi-device,id=scsi "\ > > + "-drive file={},format=raw,if=none,id=hd0 "\ > > + "-device scsi-hd,drive=hd0,bootindex=0", > > +} > > class BaseVM(object): > > -GUEST_USER = "qemu" > > -GUEST_PASS = "qemupass" > > -ROOT_PASS = "qemupass" > > Don't we need these? We don't need these since we moved them up to the new DEFAULT_CONFIG. These are essentially the default values now since the user can now override these. We also handle the cases where these are accessed by existing scripts, and ensuring backwards compatibility by referencing these values in the _config (see the code in __getattr__). This actually brings up a good point that I wanted to mention. Our initial plan was to leave the existing VM scripts unchanged. We were thinking that it would also clarify things for a later patch to simply change references to ROOT_PASS, GUEST_USER/ PASS, and SSH_PUB_KEY, within the existing VM scripts (centos, openbsd, etc) to use _config, and then we could get rid of the __getattr__ change entirely. Actually, we could even put it at the end of this series too. I think I will add this change to the next version of this patch unless you think we should keep it separate? > > > > envvars = [ > > "https_proxy", > > @@ -59,19 +84,26 @@ class BaseVM(object): > > poweroff = "poweroff" > > # enable IPv6 networking > > ipv6 = True > > -def __init__(self, debug=False, vcpus=None): > > +def __init__(self, debug=False, vcpus=None, config=None): > > self._guest = None > > +# Allow input config to override defaults. > > +self._config = DEFAULT_CONFIG.copy() > > +if config != None: > > +self._config.update(config) > > self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", > > suffix=".tmp", > > dir=".")) > > atexit.register(shutil.rmtree, self._tmpdir) > > - > > +self._config['ssh_key'] = \ > > +open(self._config['ssh_key_file']).read().rstrip() > > +self._config['ssh_pub_key'] = \ > > +open(self._config['ssh_pub_key_file']).read().rstrip() > > self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") > > -open(self._ssh_key_file, "w").write(SSH_KEY) > > +open(self._ssh_key_file, "w").write(self._config['ssh_key']) > > subprocess.check_call(["chmod", "600", self._ssh_key_file]) > > > > self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") > > -open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY) > > +open(self._ssh_pub_key_file, > > "w").write(self._config['ssh_pub_key']) > > Read as a block I find this confusing: > > self._config['ssh_key'] = \ >
Re: [PATCH 6/8] tests/vm: add --boot-console switch
On Mon, 27 Jan 2020 at 07:56, Alex Bennée wrote: > > @@ -523,6 +525,10 @@ def main(vmcls, config=None): > > if args.snapshot: > > img += ",snapshot=on" > > vm.boot(img) > > +wait_boot = getattr(vm, "wait_boot", None) > > Didn't we add a __getattr method, so we can do self._config['wait_boot'] > > > +if args.boot_console and callable(wait_boot): > > +vm.console_init() > > +wait_boot() > > isn't wait_boot always callable because it's part of the basevm? My bad. Missed changing this after moving the method into the base class. Yes, we should change it to something simpler like: if args.boot_console: vm.console_init() self.wait_boot() Thanks & Regards, -Rob > > > vm.wait_ssh() > > except Exception as e: > > if isinstance(e, SystemExit) and e.code == 0: > > > -- > Alex Bennée
Re: [PATCH v2 02/12] tests/docker: better handle symlinked libs
Hi, I was looking at this patch and have a comment about the number of groups that are expected to be found by this regex. It seems like the old code expected two groups to be found otherwise it will not append the library to the found libs. def _get_so_libs(executable): libs = [] ldd_re = re.compile(r"(/.*/)(\S*)") try: ldd_output = subprocess.check_output(["ldd", executable]).decode('utf-8') for line in ldd_output.split("\n"): search = ldd_re.search(line) if search and len(search.groups()) == 2: <<< so_path = search.groups()[0] so_lib = search.groups()[1] libs.append("%s/%s" % (so_path, so_lib)) I did a bit of experimenting with output from ldd and found a few strings where the new regex seems to generate only one group for the entire path+lib rather than one group for the path and another group for the lib. $ ldd build/aarch64-softmmu/qemu-system-aarch64 __snip__ /lib/ld-linux-aarch64.so.1 (0x9c41f000) libgmodule-2.0.so.0 => /usr/lib/aarch64-linux-gnu/libgmodule-2.0.so.0 (0x9a96e000) __snip $ python3 Python 3.6.8 (default, Oct 7 2019, 12:59:55) [GCC 8.3.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import re >>> ldd_re = re.compile(r"(?:\S+ => )?(\S*) \(:?0x[0-9a-f]+\)") >>> a = "/lib/ld-linux-aarch64.so.1 (0x9c41f000)" >>> b = "libgmodule-2.0.so.0 => /usr/lib/aarch64-linux-gnu/libgmodule-2.0.so.0 >>> (0x9a96e000)" >>> ldd_re.search(a).groups() ('/lib/ld-linux-aarch64.so.1',) >>> ldd_re.search(b).groups() ('/usr/lib/aarch64-linux-gnu/libgmodule-2.0.so.0',) >>> len(ldd_re.search(a).groups()) 1 >>> len(ldd_re.search(b).groups()) 1 >>> ldd_re_old = re.compile('(/.*/)(\S*)') >>> ldd_re_old.search(a).groups() ('/lib/', 'ld-linux-aarch64.so.1') >>> ldd_re_old.search(b).groups() ('/usr/lib/aarch64-linux-gnu/', 'libgmodule-2.0.so.0') >>> len(ldd_re_old.search(a).groups()) 2 >>> len(ldd_re_old.search(b).groups()) 2 >>> Thanks & Regards, -Rob On Thu, 30 Jan 2020 at 06:40, Alex Bennée wrote: > > When we are copying we want to ensure we grab the first > resolution (the found in path section). However even that binary might > be a symlink so lets make sure we chase the symlinks to copy the right > binary to where it can be found. > > Signed-off-by: Alex Bennée > --- > tests/docker/docker.py | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > diff --git a/tests/docker/docker.py b/tests/docker/docker.py > index 31d8adf836..7dfca63fe4 100755 > --- a/tests/docker/docker.py > +++ b/tests/docker/docker.py > @@ -109,7 +109,7 @@ def _get_so_libs(executable): > ensure theright data is copied.""" > > libs = [] > -ldd_re = re.compile(r"(/.*/)(\S*)") > +ldd_re = re.compile(r"(?:\S+ => )?(\S*) \(:?0x[0-9a-f]+\)") > try: > ldd_output = subprocess.check_output(["ldd", > executable]).decode('utf-8') > for line in ldd_output.split("\n"): > @@ -145,7 +145,8 @@ def _copy_binary_with_libs(src, bin_dest, dest_dir): > if libs: > for l in libs: > so_path = os.path.dirname(l) > -_copy_with_mkdir(l, dest_dir, so_path) > +real_l = os.path.realpath(l) > +_copy_with_mkdir(real_l, dest_dir, so_path) > def _check_binfmt_misc(executable): > -- > 2.20.1 >
[PATCH v2 03/14] tests/vm: increased max timeout for vm boot.
Add change to increase timeout waiting for VM to boot. Needed for some emulation cases where it can take longer than 5 minutes to boot. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 7 +++ 1 file changed, 7 insertions(+) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index c99725b8c0..5ca445e29a 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -57,6 +57,10 @@ class BaseVM(object): poweroff = "poweroff" # enable IPv6 networking ipv6 = True +# Scale up some timeouts under TCG. +# 4 is arbitrary, but greater than 2, +# since we found we need to wait more than twice as long. +tcg_ssh_timeout_multiplier = 4 def __init__(self, debug=False, vcpus=None): self._guest = None self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", @@ -309,6 +313,9 @@ class BaseVM(object): sys.stderr.write("### %s ...\n" % text) def wait_ssh(self, seconds=300): +# Allow more time for VM to boot under TCG. +if not kvm_available(self.arch): +seconds *= self.tcg_ssh_timeout_multiplier starttime = datetime.datetime.now() endtime = starttime + datetime.timedelta(seconds=seconds) guest_up = False -- 2.17.1
[PATCH v2 00/14] tests/vm: Add support for aarch64 VMs
This is version 2 of the patch series to add support for aarch64 VMs. - Ubuntu 18.04 aarch64 VM - CentOS 8 aarch64 VM V1: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg01180.html Changes in version 2 - Most changes relate to the patch: "Add workaround to consume console". - We changed this patch to make it cleaner. - We added a ConsoleSocket, which slots in for the current console socket with the difference being that we drain this socket in the background on a callback basis. - We also made the logging of the console to file optional - Relocated the log file path and name. For example: ~/.cache/qemu-vm/ubuntu.aarch64.install.log - Made one fix for a hang issue we were seeing. - The issue was a timing problem around a reboot where the ubuntu.aarch64 script assumed the reboot guaranteed that the next successful command would occur after the reboot. - The fix is to simply make it more deterministic by shutting down the VM and restarting it instead of issuing the reboot. - Made a few changes to CentOS VM to update its dependencies properly. - We made a few changes related to latin1 vs utf-8. We found in some cases the latin1 is needed for chars coming out of the i socket which do not have a utf-8 equivalent. Robert Foley (14): tests/vm: use $(PYTHON) consistently tests/vm: Debug mode shows ssh output. tests/vm: increased max timeout for vm boot. tests/vm: give wait_ssh() option to wait for root tests/vm: Added gen_cloud_init_iso() to basevm.py tests/vm: Add workaround to consume console tests/vm: Add configuration to basevm.py tests/vm: Added configuration file support tests/vm: add --boot-console switch tests/vm: Add ability to select QEMU from current build. tests/vm: allow wait_ssh() to specify command tests/vm: Added a new script for ubuntu.aarch64. tests/vm: Added a new script for centos.aarch64. tests/vm: change scripts to use self._config python/qemu/console_socket.py | 162 python/qemu/machine.py| 12 +- tests/vm/Makefile.include | 20 +- tests/vm/aarch64vm.py | 100 ++ tests/vm/basevm.py| 294 +- tests/vm/centos | 33 +--- tests/vm/centos-8-aarch64.ks | 51 ++ tests/vm/centos.aarch64 | 224 +++ tests/vm/conf_example_aarch64.yml | 51 ++ tests/vm/conf_example_x86.yml | 50 + tests/vm/fedora | 17 +- tests/vm/freebsd | 16 +- tests/vm/netbsd | 19 +- tests/vm/openbsd | 17 +- tests/vm/ubuntu.aarch64 | 117 tests/vm/ubuntu.i386 | 37 +--- 16 files changed, 1069 insertions(+), 151 deletions(-) create mode 100644 python/qemu/console_socket.py create mode 100644 tests/vm/aarch64vm.py create mode 100644 tests/vm/centos-8-aarch64.ks create mode 100755 tests/vm/centos.aarch64 create mode 100644 tests/vm/conf_example_aarch64.yml create mode 100644 tests/vm/conf_example_x86.yml create mode 100755 tests/vm/ubuntu.aarch64 -- 2.17.1
[PATCH v2 10/14] tests/vm: Add ability to select QEMU from current build.
Added a new special variable QEMU_LOCAL=1, which will indicate to take the QEMU binary from the current build. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 4 tests/vm/basevm.py| 29 - 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index d1444c99f5..7557c154a1 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -35,6 +35,7 @@ vm-help vm-test: @echo "BOOT_CONSOLE=1- Show the console output at boot time. " @echo "LOG_CONSOLE=1 - Log console to file in: ~/.cache/qemu-vm " @echo "V=1 - Enable verbose ouput on host and guest commands" + @echo "QEMU_LOCAL=1 - Use QEMU binary local to this build." @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool" @echo "QEMU_CONFIG=/path/conf.yml - Change path to VM configuration .yml file." @@ -54,6 +55,7 @@ $(IMAGES_DIR)/%.img: $(SRC_PATH)/tests/vm/% \ $(if $(V)$(DEBUG), --debug) \ $(if $(LOG_CONSOLE),--log-console) \ $(if $(BOOT_CONSOLE),--boot-console) \ + --build-path $(BUILD_DIR)\ --image "$@" \ --force \ --build-image $@, \ @@ -70,6 +72,7 @@ vm-build-%: $(IMAGES_DIR)/%.img $(if $(V),--verbose) \ $(if $(LOG_CONSOLE),--log-console) \ $(if $(BOOT_CONSOLE),--boot-console) \ + --build-path $(BUILD_DIR)\ --image "$<" \ $(if $(BUILD_TARGET),--build-target $(BUILD_TARGET)) \ --snapshot \ @@ -92,6 +95,7 @@ vm-boot-ssh-%: $(IMAGES_DIR)/%.img $(if $(J),--jobs $(J)) \ $(if $(LOG_CONSOLE),--log-console) \ $(if $(BOOT_CONSOLE),--boot-console) \ + --build-path $(BUILD_DIR)\ --image "$<" \ --interactive \ false, \ diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index dc94d1988e..a30a641a4a 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -89,9 +89,9 @@ class BaseVM(object): # since we found we need to wait more than twice as long. tcg_ssh_timeout_multiplier = 4 def __init__(self, debug=False, vcpus=None, config=None, - log_console=False, - boot_console=None): + log_console=False, boot_console=False, build_path=None): self._guest = None +self._build_path = build_path self._boot_console = boot_console # Allow input config to override defaults. self._config = DEFAULT_CONFIG.copy() @@ -273,8 +273,8 @@ class BaseVM(object): args += self._data_args + extra_args + self._config['extra_args'] args += ["-device", "VGA"] logging.debug("QEMU args: %s", " ".join(args)) -qemu_bin = os.environ.get("QEMU", "qemu-system-" + self.arch) -guest = QEMUMachine(binary=qemu_bin, args=args, +qemu_path = get_qemu_path(self.arch, self._build_path) +guest = QEMUMachine(binary=qemu_path, args=args, console_log=self._console_log_path) guest.set_machine(self._config['machine']) guest.set_console() @@ -479,6 +479,22 @@ class BaseVM(object): stderr=self._stdout) return os.path.join(cidir, "cloud-init.iso") +def get_qemu_path(arch, build_path=None): +"""Fetch the path to the qemu binary.""" +qemu_local = os.environ.get("QEMU_LOCAL", 0) +# If QEMU environment variable set, it takes precedence +if "QEMU" in os.environ: +qemu_path = os.environ["QEMU"] +elif qemu_local: +if not build_path: +raise Exception("--build-path option required with QEMU_LOCAL") +qemu_path = os.path.join(build_path, arch + "-softmmu") +qemu_path = os.path.join(qemu_path, "qemu-system-" + arch) +else: +# Default is to use system path for qemu. +qemu_path = "qemu-system-" + arch +return qemu_path + def parse_config(config, args): """ Parse yaml config and populate our config structure. The yaml config allows the user to override the @@ -543,6 +559,8 @@ def parse_args(vmcls): "See config_example.yaml for example.") parser.add_option("--boot-console", action="store_true", help="Show console
[PATCH v2 06/14] tests/vm: Add workaround to consume console
The ConsoleSocket object provides a socket interface which will consume all arriving characters on the socket, but will provide those chars via recv() as would a regular socket. This is a workaround we found was needed since there is a known issue where QEMU will hang waiting for console characters to be consumed. We also add the option of logging the console to a file. Signed-off-by: Robert Foley --- python/qemu/console_socket.py | 162 ++ python/qemu/machine.py| 12 ++- tests/vm/Makefile.include | 4 + tests/vm/basevm.py| 24 - 4 files changed, 194 insertions(+), 8 deletions(-) create mode 100644 python/qemu/console_socket.py diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py new file mode 100644 index 00..a1f74e60ac --- /dev/null +++ b/python/qemu/console_socket.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +# +# This python module implements a ConsoleSocket object which is +# designed always drain the socket itself, and place +# the bytes into a in memory buffer for later processing. +# +# Optionally a file path can be passed in and we will also +# dump the characters to this file for debug. +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# +import asyncore +import socket +import threading +import io +import os +import sys +from collections import deque +import time +import traceback + +class ConsoleSocket(asyncore.dispatcher): + +def __init__(self, address, file=None): +self._recv_timeout_sec = 300 +self._buffer = deque() +self._asyncore_thread = None +self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) +self._sock.connect(address) +self._logfile = None +if file: +self._logfile = open(file, "w") +asyncore.dispatcher.__init__(self, sock=self._sock) +self._thread_start() +self._open = True + +def _thread_start(self): +"""Kick off a thread to wait on the asyncore.loop""" +if self._asyncore_thread is not None: +return +self._asyncore_thread = threading.Thread(target=asyncore.loop, + kwargs={'timeout':1}) +self._asyncore_thread.daemon = True +self._asyncore_thread.start() + +def handle_close(self): +"""redirect close to base class""" +# Call the base class close, but not self.close() since +# handle_close() occurs in the context of the thread which +# self.close() attempts to join. +asyncore.dispatcher.close(self) + +def close(self): +"""Close the base object and wait for the thread to terminate""" +if self._open: +self._open = False +asyncore.dispatcher.close(self) +if self._asyncore_thread is not None: +thread, self._asyncore_thread = self._asyncore_thread, None +thread.join() +if self._logfile: +self._logfile.close() +self._logfile = None + +def handle_read(self): +"""process arriving characters into in memory _buffer""" +try: +data = asyncore.dispatcher.recv(self, 1) +# latin1 is needed since there are some chars +# we are receiving that cannot be encoded to utf-8 +# such as 0xe2, 0x80, 0xA6. +string = data.decode("latin1") +except: +print("Exception seen.") +traceback.print_exc() +return +if self._logfile: +self._logfile.write("{}".format(string)) +self._logfile.flush() +for c in string: +self._buffer.append(c) + +def recv(self, n=1): +"""Return chars from in memory buffer""" +start_time = time.time() +while len(self._buffer) < n: +time.sleep(0.1) +elapsed_sec = time.time() - start_time +if elapsed_sec > self._recv_timeout_sec: +raise socket.timeout +chars = ''.join([self._buffer.popleft() for i in range(n)]) +# We choose to use latin1 to remain consistent with +# handle_read() and give back the same data as the user would +# receive if they were reading directly from the +# socket w/o our intervention. +return chars.encode("latin1") + +def set_blocking(self): +"""Maintain compatibility with socket API""" +pass + +def settimeout(self, seconds): +"""Set current timeout on recv""" +self._recv_timeout
[PATCH v2 05/14] tests/vm: Added gen_cloud_init_iso() to basevm.py
This method was located in both centos and ubuntu.i386. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 40 tests/vm/centos | 33 + tests/vm/ubuntu.i386 | 37 + 3 files changed, 42 insertions(+), 68 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 7f26892268..8400b0e07f 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -345,6 +345,46 @@ class BaseVM(object): def qmp(self, *args, **kwargs): return self._guest.qmp(*args, **kwargs) +def gen_cloud_init_iso(self): +cidir = self._tmpdir +mdata = open(os.path.join(cidir, "meta-data"), "w") +name = self.name.replace(".","-") +mdata.writelines(["instance-id: {}-vm-0\n".format(name), + "local-hostname: {}-guest\n".format(name)]) +mdata.close() +udata = open(os.path.join(cidir, "user-data"), "w") +print("guest user:pw {}:{}".format(self._config['guest_user'], + self._config['guest_pass'])) +udata.writelines(["#cloud-config\n", + "chpasswd:\n", + " list: |\n", + "root:%s\n" % self._config['root_pass'], + "%s:%s\n" % (self._config['guest_user'], + self._config['guest_pass']), + " expire: False\n", + "users:\n", + " - name: %s\n" % self._config['guest_user'], + "sudo: ALL=(ALL) NOPASSWD:ALL\n", + "ssh-authorized-keys:\n", + "- %s\n" % self._config['ssh_pub_key'], + " - name: root\n", + "ssh-authorized-keys:\n", + "- %s\n" % self._config['ssh_pub_key'], + "locale: en_US.UTF-8\n"]) +proxy = os.environ.get("http_proxy") +if not proxy is None: +udata.writelines(["apt:\n", + " proxy: %s" % proxy]) +udata.close() +subprocess.check_call(["genisoimage", "-output", "cloud-init.iso", + "-volid", "cidata", "-joliet", "-rock", + "user-data", "meta-data"], + cwd=cidir, + stdin=self._devnull, stdout=self._stdout, + stderr=self._stdout) + +return os.path.join(cidir, "cloud-init.iso") + def parse_args(vmcls): def get_default_jobs(): diff --git a/tests/vm/centos b/tests/vm/centos index a41ff109eb..0ad4ecf419 100755 --- a/tests/vm/centos +++ b/tests/vm/centos @@ -31,37 +31,6 @@ class CentosVM(basevm.BaseVM): make docker-test-mingw@fedora {verbose} J={jobs} NETWORK=1; """ -def _gen_cloud_init_iso(self): -cidir = self._tmpdir -mdata = open(os.path.join(cidir, "meta-data"), "w") -mdata.writelines(["instance-id: centos-vm-0\n", - "local-hostname: centos-guest\n"]) -mdata.close() -udata = open(os.path.join(cidir, "user-data"), "w") -udata.writelines(["#cloud-config\n", - "chpasswd:\n", - " list: |\n", - "root:%s\n" % self.ROOT_PASS, - "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS), - " expire: False\n", - "users:\n", - " - name: %s\n" % self.GUEST_USER, - "sudo: ALL=(ALL) NOPASSWD:ALL\n", - "ssh-authorized-keys:\n", - "- %s\n" % basevm.SSH_PUB_KEY, - " - name: root\n", - "ssh-authorized-keys:\n", - "- %s\n" % basevm.SSH_PUB_KEY, - "locale: en_US.UTF-8\n"]) -udata.close() -subprocess.check_call(["genisoimage", "-output", "cloud-init.iso", - "-v
[PATCH v2 08/14] tests/vm: Added configuration file support
Changes to tests/vm/basevm.py to allow accepting a configuration file as a parameter. Allows for specifying VM options such as cpu, machine, memory, and arbitrary qemu arguments for specifying options such as NUMA configuration. Also added an example conf_example_aarch64.yml and conf_example_x86.yml. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 2 ++ tests/vm/basevm.py| 29 +- tests/vm/conf_example_aarch64.yml | 51 +++ tests/vm/conf_example_x86.yml | 50 ++ 4 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 tests/vm/conf_example_aarch64.yml create mode 100644 tests/vm/conf_example_x86.yml diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 76a1bd7fe8..e19072f303 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -36,6 +36,8 @@ vm-help vm-test: @echo "V=1 - Enable verbose ouput on host and guest commands" @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool" + @echo "QEMU_CONFIG=/path/conf.yml - Change path to VM configuration .yml file." + @echo " See conf_example_*.yml for file format details." vm-build-all: $(addprefix vm-build-, $(IMAGES)) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index ed1b56bcea..a24ce090c7 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -29,6 +29,7 @@ import tempfile import shutil import multiprocessing import traceback +import yaml SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), "..", "keys", "id_rsa") @@ -474,9 +475,31 @@ class BaseVM(object): cwd=cidir, stdin=self._devnull, stdout=self._stdout, stderr=self._stdout) - return os.path.join(cidir, "cloud-init.iso") +def parse_config(config, args): +""" Parse yaml config and populate our config structure. +The yaml config allows the user to override the +defaults for VM parameters. In many cases these +defaults can be overridden without rebuilding the VM.""" +if args.config: +config_file = args.config +elif 'QEMU_CONFIG' in os.environ: +config_file = os.environ['QEMU_CONFIG'] +else: +return config +if not os.path.exists(config_file): +raise Exception("config file {} does not exist".format(config_file)) +with open(config_file) as f: +yaml_dict = yaml.safe_load(f) + +if 'qemu-conf' in yaml_dict: +config.update(yaml_dict['qemu-conf']) +else: +raise Exception("config file {} is not valid"\ +" missing qemu-conf".format(config_file)) +return config + def parse_args(vmcls): def get_default_jobs(): @@ -513,6 +536,9 @@ def parse_args(vmcls): help="run tests with a snapshot") parser.add_option("--log-console", action="store_true", help="Log console to file.") +parser.add_option("--config", "-c", default=None, + help="Provide config yaml for configuration. "\ + "See config_example.yaml for example.") parser.disable_interspersed_args() return parser.parse_args() @@ -524,6 +550,7 @@ def main(vmcls, config=None): if not argv and not args.build_qemu and not args.build_image: print("Nothing to do?") return 1 +config = parse_config(config, args) logging.basicConfig(level=(logging.DEBUG if args.debug else logging.WARN)) vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config, diff --git a/tests/vm/conf_example_aarch64.yml b/tests/vm/conf_example_aarch64.yml new file mode 100644 index 00..9d44ae356f --- /dev/null +++ b/tests/vm/conf_example_aarch64.yml @@ -0,0 +1,51 @@ +# +# Example yaml for use by any of the scripts in tests/vm. +# Can be provided as an environment variable QEMU_CONFIG +# +qemu-conf: + +# If any of the below are not provided, we will just use the qemu defaults. + +# Login username and password(has to be sudo enabled) +guest_user: qemu +guest_pass: "qemupass" + +# Password for root user can be different from guest. +root_pass: "qemupass" + +# If one key is provided, both must be provided. +#ssh_key: /complete/path/of/your/keyfile/id_rsa +#ssh_pub_key: /complete/path/of/your/keyfile/id_rsa.pub + +cpu: max +machine: virt,gic-versio
[PATCH v2 09/14] tests/vm: add --boot-console switch
Added ability to view console during boot via --boot-console switch to basevm.py. This helps debug issues that occur during the boot sequence. Also added a new special variable to vm-build: BOOT_CONSOLE=1 will cause this new --boot-console switch to be set. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 4 tests/vm/basevm.py| 11 +-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index e19072f303..d1444c99f5 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -32,6 +32,7 @@ vm-help vm-test: @echo 'EXTRA_CONFIGURE_OPTS="..."' @echo "J=[0..9]* - Override the -jN parameter for make commands" @echo "DEBUG=1 - Enable verbose output on host and interactive debugging" + @echo "BOOT_CONSOLE=1- Show the console output at boot time. " @echo "LOG_CONSOLE=1 - Log console to file in: ~/.cache/qemu-vm " @echo "V=1 - Enable verbose ouput on host and guest commands" @echo "QEMU=/path/to/qemu- Change path to QEMU binary" @@ -52,6 +53,7 @@ $(IMAGES_DIR)/%.img: $(SRC_PATH)/tests/vm/% \ $(PYTHON) $< \ $(if $(V)$(DEBUG), --debug) \ $(if $(LOG_CONSOLE),--log-console) \ + $(if $(BOOT_CONSOLE),--boot-console) \ --image "$@" \ --force \ --build-image $@, \ @@ -67,6 +69,7 @@ vm-build-%: $(IMAGES_DIR)/%.img $(if $(J),--jobs $(J)) \ $(if $(V),--verbose) \ $(if $(LOG_CONSOLE),--log-console) \ + $(if $(BOOT_CONSOLE),--boot-console) \ --image "$<" \ $(if $(BUILD_TARGET),--build-target $(BUILD_TARGET)) \ --snapshot \ @@ -88,6 +91,7 @@ vm-boot-ssh-%: $(IMAGES_DIR)/%.img $(PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(J),--jobs $(J)) \ $(if $(LOG_CONSOLE),--log-console) \ + $(if $(BOOT_CONSOLE),--boot-console) \ --image "$<" \ --interactive \ false, \ diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index a24ce090c7..dc94d1988e 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -89,8 +89,10 @@ class BaseVM(object): # since we found we need to wait more than twice as long. tcg_ssh_timeout_multiplier = 4 def __init__(self, debug=False, vcpus=None, config=None, - log_console=False): + log_console=False, + boot_console=None): self._guest = None +self._boot_console = boot_console # Allow input config to override defaults. self._config = DEFAULT_CONFIG.copy() if config != None: @@ -539,6 +541,8 @@ def parse_args(vmcls): parser.add_option("--config", "-c", default=None, help="Provide config yaml for configuration. "\ "See config_example.yaml for example.") +parser.add_option("--boot-console", action="store_true", + help="Show console during boot. ") parser.disable_interspersed_args() return parser.parse_args() @@ -554,7 +558,8 @@ def main(vmcls, config=None): logging.basicConfig(level=(logging.DEBUG if args.debug else logging.WARN)) vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config, - log_console=args.log_console) + log_console=args.log_console, + boot_console=args.boot_console) if args.build_image: if os.path.exists(args.image) and not args.force: sys.stderr.writelines(["Image file exists: %s\n" % args.image, @@ -574,6 +579,8 @@ def main(vmcls, config=None): if args.snapshot: img += ",snapshot=on" vm.boot(img) +if vm._boot_console: +vm.wait_boot() vm.wait_ssh() except Exception as e: if isinstance(e, SystemExit) and e.code == 0: -- 2.17.1
[PATCH v2 12/14] tests/vm: Added a new script for ubuntu.aarch64.
ubuntu.aarch64 provides a script to create an Ubuntu 18.04 VM. Another new file is also added aarch64vm.py, which is a module with common methods used by aarch64 VMs, such as how to create the flash images. Signed-off-by: Robert Foley --- tests/vm/Makefile.include | 3 +- tests/vm/aarch64vm.py | 100 tests/vm/basevm.py| 8 +++ tests/vm/ubuntu.aarch64 | 117 ++ 4 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 tests/vm/aarch64vm.py create mode 100755 tests/vm/ubuntu.aarch64 diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 7557c154a1..9dfa517936 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -2,7 +2,7 @@ .PHONY: vm-build-all vm-clean-all -IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 IMAGES_DIR := $(HOME)/.cache/qemu-vm/images IMAGE_FILES := $(patsubst %, $(IMAGES_DIR)/%.img, $(IMAGES)) @@ -18,6 +18,7 @@ vm-help vm-test: @echo " vm-build-openbsd- Build QEMU in OpenBSD VM" @echo " vm-build-centos - Build QEMU in CentOS VM, with Docker" @echo " vm-build-fedora - Build QEMU in Fedora VM" + @echo " vm-build-ubuntu.aarch64 - Build QEMU in ubuntu aarch64 VM" @echo "" @echo " vm-build-all- Build QEMU in all VMs" @echo " vm-clean-all- Clean up VM images" diff --git a/tests/vm/aarch64vm.py b/tests/vm/aarch64vm.py new file mode 100644 index 00..fd48735b3a --- /dev/null +++ b/tests/vm/aarch64vm.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python3 +# +# VM testing aarch64 library +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# +import os +import sys +import subprocess +import basevm +from qemu.accel import kvm_available + +# This is the config needed for current version of QEMU. +# This works for both kvm and tcg. +CURRENT_CONFIG = { +'cpu' : "max", +'machine' : "virt,gic-version=max", +} + +# The minimum minor version of QEMU we will support with aarch64 VMs is 3. +# QEMU versions less than 3 have various issues running these VMs. +QEMU_AARCH64_MIN_VERSION = 3 + +# The DEFAULT_CONFIG will default to a version of +# parameters that works for backwards compatibility. +DEFAULT_CONFIG = {'kvm' : {'cpu' : "host", + 'machine' : "virt,gic-version=host"}, + 'tcg' : {'cpu' : "cortex-a57", + 'machine' : "virt"}, +} + +def get_config_defaults(vmcls, default_config): +"""Fetch the configuration defaults for this VM, + taking into consideration the defaults for + aarch64 first, followed by the defaults for this VM.""" +config = default_config +config.update(aarch_get_config_defaults(vmcls)) +return config + +def aarch_get_config_defaults(vmcls): +# Set the defaults for current version of QEMU. +config = CURRENT_CONFIG +args, argv = basevm.parse_args(vmcls) +qemu_path = basevm.get_qemu_path(vmcls.arch, args.build_path) +qemu_version = basevm.get_qemu_version(qemu_path) +if qemu_version < QEMU_AARCH64_MIN_VERSION: +error = "\nThis major version of QEMU {} is to old for aarch64 VMs.\n"\ +"The major version must be at least {}.\n"\ +"To continue with the current build of QEMU, "\ +"please restart with QEMU_LOCAL=1 .\n" +print(error.format(qemu_version, QEMU_AARCH64_MIN_VERSION)) +exit(1) +if qemu_version == QEMU_AARCH64_MIN_VERSION: +# We have an older version of QEMU, +# set the config values for backwards compatibility. +if kvm_available('aarch64'): +config.update(DEFAULT_CONFIG['kvm']) +else: +config.update(DEFAULT_CONFIG['tcg']) +return config + +def create_flash_images(flash_dir="./"): +"""Creates the appropriate pflash files + for an aarch64 VM.""" +flash0_path = get_flash_path(flash_dir, "flash0") +flash1_path = get_flash_path(flash_dir, "flash1") +subprocess.check_call(["dd", "if=/dev/zero", "of={}".format(flash0_path), + "bs=1M", "count=64"]) +# A reliable way to get the QEMU EFI image is via an installed package. +efi_img = "/usr/share/qemu-efi-aarch64/QEMU_EFI.fd" +if not os.path.exists(e
[PATCH v2 14/14] tests/vm: change scripts to use self._config
This change converts existing scripts to using for example self.ROOT_PASS, to self._config['root_pass']. We made similar changes for GUEST_USER, and GUEST_PASS. This allows us also to remove the change in basevm.py, which adds __getattr__ for backwards compatibility. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 11 ++- tests/vm/fedora| 17 + tests/vm/freebsd | 16 tests/vm/netbsd| 19 ++- tests/vm/openbsd | 17 + 5 files changed, 38 insertions(+), 42 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 6ed04a2a4b..ad04789bcf 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -178,13 +178,6 @@ class BaseVM(object): self.console_init(timeout=timeout) self.console_wait(wait_string) -def __getattr__(self, name): -# Support direct access to config by key. -# for example, access self._config['cpu'] by self.cpu -if name.lower() in self._config.keys(): -return self._config[name.lower()] -return object.__getattribute__(self, name) - def _download_with_cache(self, url, sha256sum=None, sha512sum=None): def check_sha256sum(fname): if not sha256sum: @@ -234,13 +227,13 @@ class BaseVM(object): return r def ssh(self, *cmd): -return self._ssh_do(self.GUEST_USER, cmd, False) +return self._ssh_do(self._config["guest_user"], cmd, False) def ssh_root(self, *cmd): return self._ssh_do("root", cmd, False) def ssh_check(self, *cmd): -self._ssh_do(self.GUEST_USER, cmd, True) +self._ssh_do(self._config["guest_user"], cmd, True) def ssh_root_check(self, *cmd): self._ssh_do("root", cmd, True) diff --git a/tests/vm/fedora b/tests/vm/fedora index 4d7d6049f4..60195ddc46 100755 --- a/tests/vm/fedora +++ b/tests/vm/fedora @@ -105,20 +105,20 @@ class FedoraVM(basevm.BaseVM): self.console_wait_send("7) [!] Root password", "7\n") self.console_wait("Password:") -self.console_send("%s\n" % self.ROOT_PASS) +self.console_send("%s\n" % self._config["root_pass"]) self.console_wait("Password (confirm):") -self.console_send("%s\n" % self.ROOT_PASS) +self.console_send("%s\n" % self._config["root_pass"]) self.console_wait_send("8) [ ] User creation", "8\n") self.console_wait_send("1) [ ] Create user", "1\n") self.console_wait_send("3) User name", "3\n") -self.console_wait_send("ENTER:", "%s\n" % self.GUEST_USER) +self.console_wait_send("ENTER:", "%s\n" % self._config["guest_user"]) self.console_wait_send("4) [ ] Use password", "4\n") self.console_wait_send("5) Password", "5\n") self.console_wait("Password:") -self.console_send("%s\n" % self.GUEST_PASS) +self.console_send("%s\n" % self._config["guest_pass"]) self.console_wait("Password (confirm):") -self.console_send("%s\n" % self.GUEST_PASS) +self.console_send("%s\n" % self._config["guest_pass"]) self.console_wait_send("7) Groups","c\n") while True: @@ -136,7 +136,7 @@ class FedoraVM(basevm.BaseVM): if good: break time.sleep(10) -self.console_send("r\n" % self.GUEST_PASS) +self.console_send("r\n" % self._config["guest_pass"]) self.console_wait_send("'b' to begin install", "b\n") @@ -147,12 +147,13 @@ class FedoraVM(basevm.BaseVM): # setup qemu user prompt = " ~]$" -self.console_ssh_init(prompt, self.GUEST_USER, self.GUEST_PASS) +self.console_ssh_init(prompt, self._config["guest_user"], + self._config["guest_pass"]) self.console_wait_send(prompt, "exit\n") # setup root user prompt = " ~]#" -self.console_ssh_init(prompt, "root", self.ROOT_PASS) +self.console_ssh_init(prompt, "root", self._config["root_pass"]) self.console_sshd_config(prompt) # setup virtio-blk #1 (tarfile) diff --git a/tests/vm/freebsd b/tests/vm/freebsd index fb54334696..735fb5f794 100755 --- a/tests/vm/freebsd +++ b/tests/vm/freebsd @@ -110,9 +110,9 @@ class FreeBSDVM(basevm.BaseVM): # post-install configuration
[PATCH v2 11/14] tests/vm: allow wait_ssh() to specify command
This allows for waiting for completion of arbitrary commands. Signed-off-by: Robert Foley --- tests/vm/basevm.py | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index a30a641a4a..792e4a3fb2 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -407,24 +407,24 @@ class BaseVM(object): def print_step(self, text): sys.stderr.write("### %s ...\n" % text) -def wait_ssh(self, wait_root=False, seconds=300): +def wait_ssh(self, wait_root=False, seconds=300, cmd="exit 0"): # Allow more time for VM to boot under TCG. if not kvm_available(self.arch): seconds *= self.tcg_ssh_timeout_multiplier starttime = datetime.datetime.now() endtime = starttime + datetime.timedelta(seconds=seconds) -guest_up = False +cmd_success = False while datetime.datetime.now() < endtime: -if wait_root and self.ssh_root("exit 0") == 0: -guest_up = True +if wait_root and self.ssh_root(cmd) == 0: +cmd_success = True break -elif self.ssh("exit 0") == 0: -guest_up = True +elif self.ssh(cmd) == 0: +cmd_success = True break seconds = (endtime - datetime.datetime.now()).total_seconds() logging.debug("%ds before timeout", seconds) time.sleep(1) -if not guest_up: +if not cmd_success: raise Exception("Timeout while waiting for guest ssh") def shutdown(self): -- 2.17.1
[PATCH v2 02/14] tests/vm: Debug mode shows ssh output.
Add changes to tests/vm/basevm.py so that during debug mode we show ssh output. Signed-off-by: Robert Foley Reviewed-by: Peter Puhov Reviewed-by: Alex Bennée --- tests/vm/basevm.py | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 4dee6647e6..c99725b8c0 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -120,11 +120,16 @@ class BaseVM(object): return fname def _ssh_do(self, user, cmd, check): -ssh_cmd = ["ssh", "-q", "-t", +ssh_cmd = ["ssh", + "-t", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=" + os.devnull, "-o", "ConnectTimeout=1", "-p", self.ssh_port, "-i", self._ssh_key_file] +# If not in debug mode, set ssh to quiet mode to +# avoid printing the results of commands. +if not self.debug: +ssh_cmd.append("-q") for var in self.envvars: ssh_cmd += ['-o', "SendEnv=%s" % var ] assert not isinstance(cmd, str) -- 2.17.1
[PATCH v2 04/14] tests/vm: give wait_ssh() option to wait for root
Allow wait_ssh to wait for root user to be ready. This solves the issue where we perform a wait_ssh() successfully, but the root user is not yet ready to be logged in. Signed-off-by: Robert Foley Reviewed-by: Alex Bennée Reviewed-by: Peter Puhov --- tests/vm/basevm.py | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 5ca445e29a..7f26892268 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -312,7 +312,7 @@ class BaseVM(object): def print_step(self, text): sys.stderr.write("### %s ...\n" % text) -def wait_ssh(self, seconds=300): +def wait_ssh(self, wait_root=False, seconds=300): # Allow more time for VM to boot under TCG. if not kvm_available(self.arch): seconds *= self.tcg_ssh_timeout_multiplier @@ -320,7 +320,10 @@ class BaseVM(object): endtime = starttime + datetime.timedelta(seconds=seconds) guest_up = False while datetime.datetime.now() < endtime: -if self.ssh("exit 0") == 0: +if wait_root and self.ssh_root("exit 0") == 0: +guest_up = True +break +elif self.ssh("exit 0") == 0: guest_up = True break seconds = (endtime - datetime.datetime.now()).total_seconds() -- 2.17.1
[PATCH v2 13/14] tests/vm: Added a new script for centos.aarch64.
centos.aarch64 creates a CentOS 8 image. Also added a new kickstart script used to build the centos.aarch64 image. Signed-off-by: Robert Foley --- tests/vm/Makefile.include| 3 +- tests/vm/centos-8-aarch64.ks | 51 tests/vm/centos.aarch64 | 224 +++ 3 files changed, 277 insertions(+), 1 deletion(-) create mode 100644 tests/vm/centos-8-aarch64.ks create mode 100755 tests/vm/centos.aarch64 diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index 9dfa517936..bb41bc255f 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -2,7 +2,7 @@ .PHONY: vm-build-all vm-clean-all -IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos fedora ubuntu.aarch64 centos.aarch64 IMAGES_DIR := $(HOME)/.cache/qemu-vm/images IMAGE_FILES := $(patsubst %, $(IMAGES_DIR)/%.img, $(IMAGES)) @@ -19,6 +19,7 @@ vm-help vm-test: @echo " vm-build-centos - Build QEMU in CentOS VM, with Docker" @echo " vm-build-fedora - Build QEMU in Fedora VM" @echo " vm-build-ubuntu.aarch64 - Build QEMU in ubuntu aarch64 VM" + @echo " vm-build-centos.aarch64 - Build QEMU in CentOS aarch64 VM" @echo "" @echo " vm-build-all- Build QEMU in all VMs" @echo " vm-clean-all- Clean up VM images" diff --git a/tests/vm/centos-8-aarch64.ks b/tests/vm/centos-8-aarch64.ks new file mode 100644 index 00..fd6ebe4d49 --- /dev/null +++ b/tests/vm/centos-8-aarch64.ks @@ -0,0 +1,51 @@ +# CentOS aarch64 image kickstart file. +# This file is used by the CentOS installer to +# script the generation of the image. +# +# Copyright 2020 Linaro +# +ignoredisk --only-use=vda +# System bootloader configuration +bootloader --append=" crashkernel=auto" --location=mbr --boot-drive=vda +autopart --type=plain +# Partition clearing information +clearpart --linux --initlabel --drives=vda +# Use text mode install +text +repo --name="AppStream" --baseurl=file:///run/install/repo/AppStream +# Use CDROM installation media +cdrom +# Keyboard layouts +keyboard --vckeymap=us --xlayouts='' +# System language +lang en_US.UTF-8 + +# Network information +network --bootproto=dhcp --device=enp0s1 --onboot=off --ipv6=auto --no-activate +network --hostname=localhost.localdomain +# Run the Setup Agent on first boot +firstboot --enable +# Do not configure the X Window System +skipx +# System services +services --enabled="chronyd" +# System timezone +timezone America/New_York --isUtc + +# Shutdown after installation is complete. +shutdown + +%packages +@^server-product-environment +kexec-tools + +%end + +%addon com_redhat_kdump --enable --reserve-mb='auto' + +%end +%anaconda +pwpolicy root --minlen=6 --minquality=1 --notstrict --nochanges --notempty +pwpolicy user --minlen=6 --minquality=1 --notstrict --nochanges --emptyok +pwpolicy luks --minlen=6 --minquality=1 --notstrict --nochanges --notempty +%end diff --git a/tests/vm/centos.aarch64 b/tests/vm/centos.aarch64 new file mode 100755 index 00..c4a020dc4c --- /dev/null +++ b/tests/vm/centos.aarch64 @@ -0,0 +1,224 @@ +#!/usr/bin/env python3 +# +# Centos aarch64 image +# +# Copyright 2020 Linaro +# +# Authors: +# Robert Foley +# Originally based on ubuntu.aarch64 +# +# This code is licensed under the GPL version 2 or later. See +# the COPYING file in the top-level directory. +# + +import os +import sys +import subprocess +import basevm +import time +import traceback +import aarch64vm + +DEFAULT_CONFIG = { +'cpu' : "max", +'machine' : "virt,gic-version=max", +'install_cmds' : "yum install -y make git python3 gcc gcc-c++ flex bison, "\ +"yum install -y glib2-devel pixman-devel zlib-devel, "\ +"yum install -y perl-Test-Harness, "\ +"sudo dnf config-manager "\ +"--add-repo=https://download.docker.com/linux/centos/docker-ce.repo,"\ +"sudo dnf install --nobest -y docker-ce.aarch64,"\ +"systemctl enable docker", +# We increase beyond the default time since during boot +# it can take some time (many seconds) to log into the VM. +'ssh_timeout' : 60, +} + +class CentosAarch64VM(basevm.BaseVM): +name = "centos.aarch64" +arch = "aarch64" +login_prompt = "localhost login:" +prompt = '[root@localhost ~]#' +image_name = "CentOS-8-aarch64-1905-dvd1.iso" +image_link = "http://mirrors.usc.edu/pub/linux/distributions/centos/8.0.1905/isos/aarch64/; +image_link += image_name +BUILD_SCRIPT = """ +set -e; +cd $(mktemp -d); +sudo chmod a+r /dev/vdb; +
Re: [PATCH v1 08/14] tests/vm: Added configuration file support
On Fri, 14 Feb 2020 at 11:54, Alex Bennée wrote: > > from socket_thread import SocketThread > > +import yaml > > So this throws my setup on my Gentoo SynQuacer. Is this meant to be in > the standard library or is this a separate dependency? > This is a separate dependency. On Ubuntu the package is python3-yaml. This brings up an interesting point. If the yaml dependency is not there, should we expect to gracefully handle this in the python script, and simply not allow yaml files? I suppose we could error out if a yaml file is supplied and ask for the dependency to get installed. Or is there something we should do earlier, maybe in the configure, to warn or error about the missing dependency? Thanks & Regards, -Rob > -- > Alex Bennée
Re: [PATCH v2 09/19] tracing: only allow -trace to override -D if set
On Thu, 13 Feb 2020 at 17:51, Alex Bennée wrote: > > Otherwise any -D settings the user may have made get ignored. > > Signed-off-by: Alex Bennée Reviewed-by: Robert Foley