On 09/20/2018 12:31 PM, Peter Xu wrote:
On Wed, Sep 19, 2018 at 09:35:20PM +0800, Fei Li wrote:Add judgement in compress_threads_save_cleanup() to check whether the static CompressParam *comp_param has been allocated. If not, just return; or else Segmentation fault will occur when using the NULL comp_param's parameters in terminate_compression_threads(). One test case can reproduce this error is: set the compression on and migrate to a wrong nonexistent host IP address. Add judgement before handling comp_param[idx]'s quit and cond in terminate_compression_threads(), in case they are not initialized. Or else "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will occur. One test case can reproduce this error is: set the compression on and fail to fully setup the eight compression thread in compress_threads_save_setup(). Signed-off-by: Fei Li <f...@suse.com> --- migration/ram.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/migration/ram.c b/migration/ram.c index 79c89425a3..522a5550b4 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -427,6 +427,9 @@ static inline void terminate_compression_threads(void) thread_count = migrate_compress_threads();for (idx = 0; idx < thread_count; idx++) {+ if (!comp_param[idx].mutex.initialized) { + break; + }Instead of accessing the mutex internal variable "initialized", how about we just squash the terminate_compression_threads() into existing compress_threads_save_cleanup()? Note that we have this in compress_threads_save_cleanup() already: for (i = 0; i < thread_count; i++) { /* * we use it as a indicator which shows if the thread is * properly init'd or not */ if (!comp_param[i].file) { break; } qemu_thread_join(compress_threads + i); qemu_mutex_destroy(&comp_param[i].mutex); qemu_cond_destroy(&comp_param[i].cond); deflateEnd(&comp_param[i].stream); g_free(comp_param[i].originbuf); qemu_fclose(comp_param[i].file); comp_param[i].file = NULL; } So we already try to detect this using the comp_param[i].file, which IMO is better (the exposure of the mutex.init var is just "an accident" - logically we could hide that from mutex impl).
Actually, I firstly use the comp_param[i].file as the judging condition, but I am not sure whether the comp_param[i].file is also needed to be protected by the mutex lock.. And I have no objection to the squash.
qemu_mutex_lock(&comp_param[idx].mutex); comp_param[idx].quit = true; qemu_cond_signal(&comp_param[idx].cond); @@ -438,7 +441,7 @@ static void compress_threads_save_cleanup(void) { int i, thread_count;- if (!migrate_use_compression()) {+ if (!migrate_use_compression() || !comp_param) {This should be a valid fix for a crash (since we might call the cleanup code even without setup when connect() never worked, so we'd better handle it well). Considering that this seems to fix a migration crash on source, I'm CCing Dave and Juan in case this (or a new version) could even be a good candidate as part of a migration pull. Thanks,
Thanks for helping cc. ;) Have a nice day Fei
return; } terminate_compression_threads(); -- 2.13.7
