Message ID | 20180925091440.18910-1-fli@suse.com |
---|---|
State | New |
Headers | show |
Series | migration: fix the compression code | expand |
On Tue, Sep 25, 2018 at 05:14:40PM +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. One test case can reproduce this is: set > the compression on and migrate to a wrong nonexistent host IP address. > > Our current code does not judge before handling comp_param[idx]'s quit > and cond that whether they have been initialized. If not initialized, > "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will > occur. Fix this by squashing the terminate_compression_threads() into > compress_threads_save_cleanup() and employing the existing judgement > condition. One test case can reproduce this error is: set the > compression on and fail to fully setup the default eight compression > thread in compress_threads_save_setup(). > > Signed-off-by: Fei Li <fli@suse.com> Reviewed-by: Peter Xu <peterx@redhat.com> Regards,
* Fei Li (fli@suse.com) 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. One test case can reproduce this is: set > the compression on and migrate to a wrong nonexistent host IP address. > > Our current code does not judge before handling comp_param[idx]'s quit > and cond that whether they have been initialized. If not initialized, > "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will > occur. Fix this by squashing the terminate_compression_threads() into > compress_threads_save_cleanup() and employing the existing judgement > condition. One test case can reproduce this error is: set the > compression on and fail to fully setup the default eight compression > thread in compress_threads_save_setup(). > > Signed-off-by: Fei Li <fli@suse.com> Queued. > --- > migration/ram.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index f6fd8e5e09..050d535068 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -420,28 +420,14 @@ static void *do_data_compress(void *opaque) > return NULL; > } > > -static inline void terminate_compression_threads(void) > -{ > - int idx, thread_count; > - > - thread_count = migrate_compress_threads(); > - > - for (idx = 0; idx < thread_count; idx++) { > - qemu_mutex_lock(&comp_param[idx].mutex); > - comp_param[idx].quit = true; > - qemu_cond_signal(&comp_param[idx].cond); > - qemu_mutex_unlock(&comp_param[idx].mutex); > - } > -} > - > static void compress_threads_save_cleanup(void) > { > int i, thread_count; > > - if (!migrate_use_compression()) { > + if (!migrate_use_compression() || !comp_param) { > return; > } > - terminate_compression_threads(); > + > thread_count = migrate_compress_threads(); > for (i = 0; i < thread_count; i++) { > /* > @@ -451,6 +437,12 @@ static void compress_threads_save_cleanup(void) > if (!comp_param[i].file) { > break; > } > + > + qemu_mutex_lock(&comp_param[i].mutex); > + comp_param[i].quit = true; > + qemu_cond_signal(&comp_param[i].cond); > + qemu_mutex_unlock(&comp_param[i].mutex); > + > qemu_thread_join(compress_threads + i); > qemu_mutex_destroy(&comp_param[i].mutex); > qemu_cond_destroy(&comp_param[i].cond); > -- > 2.13.7 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/ram.c b/migration/ram.c index f6fd8e5e09..050d535068 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -420,28 +420,14 @@ static void *do_data_compress(void *opaque) return NULL; } -static inline void terminate_compression_threads(void) -{ - int idx, thread_count; - - thread_count = migrate_compress_threads(); - - for (idx = 0; idx < thread_count; idx++) { - qemu_mutex_lock(&comp_param[idx].mutex); - comp_param[idx].quit = true; - qemu_cond_signal(&comp_param[idx].cond); - qemu_mutex_unlock(&comp_param[idx].mutex); - } -} - static void compress_threads_save_cleanup(void) { int i, thread_count; - if (!migrate_use_compression()) { + if (!migrate_use_compression() || !comp_param) { return; } - terminate_compression_threads(); + thread_count = migrate_compress_threads(); for (i = 0; i < thread_count; i++) { /* @@ -451,6 +437,12 @@ static void compress_threads_save_cleanup(void) if (!comp_param[i].file) { break; } + + qemu_mutex_lock(&comp_param[i].mutex); + comp_param[i].quit = true; + qemu_cond_signal(&comp_param[i].cond); + qemu_mutex_unlock(&comp_param[i].mutex); + qemu_thread_join(compress_threads + i); qemu_mutex_destroy(&comp_param[i].mutex); qemu_cond_destroy(&comp_param[i].cond);
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. One test case can reproduce this is: set the compression on and migrate to a wrong nonexistent host IP address. Our current code does not judge before handling comp_param[idx]'s quit and cond that whether they have been initialized. If not initialized, "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will occur. Fix this by squashing the terminate_compression_threads() into compress_threads_save_cleanup() and employing the existing judgement condition. One test case can reproduce this error is: set the compression on and fail to fully setup the default eight compression thread in compress_threads_save_setup(). Signed-off-by: Fei Li <fli@suse.com> --- migration/ram.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)