| Message ID | 20240913220542.18305-2-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/multifd: Fix rb->receivedmap cleanup race | expand |
On Fri, Sep 13, 2024 at 07:05:41PM -0300, Fabiano Rosas wrote: > There are two qemu_loadvm_state_cleanup() calls that were introduced > when qemu_loadvm_state_setup() was still called before loading the > configuration section, so there was state to be cleaned up if the > header checks failed. > > However, commit 9e14b84908 ("migration/savevm: load_header before > load_setup") has moved that configuration section part to > qemu_loadvm_state_header() which now happens before > qemu_loadvm_state_setup(). > > Remove the cleanup calls that are now misplaced. > > CC: qemu-stable@nongnu.org > Fixes: 9e14b84908 ("migration/savevm: load_header before load_setup") > Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> We don't need to copy stable, am I right? IIUC it's a good cleanup, however not a bug fix, as qemu_loadvm_state_cleanup() can be invoked without calling _setup() safely? > --- > migration/savevm.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index d500eae979..d0759694fd 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2732,13 +2732,11 @@ static int qemu_loadvm_state_header(QEMUFile *f) > if (migrate_get_current()->send_configuration) { > if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > error_report("Configuration section missing"); > - qemu_loadvm_state_cleanup(); > return -EINVAL; > } > ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > > if (ret) { > - qemu_loadvm_state_cleanup(); > return ret; > } > } > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Fri, Sep 13, 2024 at 07:05:41PM -0300, Fabiano Rosas wrote: >> There are two qemu_loadvm_state_cleanup() calls that were introduced >> when qemu_loadvm_state_setup() was still called before loading the >> configuration section, so there was state to be cleaned up if the >> header checks failed. >> >> However, commit 9e14b84908 ("migration/savevm: load_header before >> load_setup") has moved that configuration section part to >> qemu_loadvm_state_header() which now happens before >> qemu_loadvm_state_setup(). >> >> Remove the cleanup calls that are now misplaced. >> >> CC: qemu-stable@nongnu.org >> Fixes: 9e14b84908 ("migration/savevm: load_header before load_setup") >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > We don't need to copy stable, am I right? IIUC it's a good cleanup, > however not a bug fix, as qemu_loadvm_state_cleanup() can be invoked > without calling _setup() safely? Hm, I think you're right. If we fail in the header part the multifd threads will still be waiting for the ram code to release them.
diff --git a/migration/savevm.c b/migration/savevm.c index d500eae979..d0759694fd 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2732,13 +2732,11 @@ static int qemu_loadvm_state_header(QEMUFile *f) if (migrate_get_current()->send_configuration) { if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { error_report("Configuration section missing"); - qemu_loadvm_state_cleanup(); return -EINVAL; } ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); if (ret) { - qemu_loadvm_state_cleanup(); return ret; } }
There are two qemu_loadvm_state_cleanup() calls that were introduced when qemu_loadvm_state_setup() was still called before loading the configuration section, so there was state to be cleaned up if the header checks failed. However, commit 9e14b84908 ("migration/savevm: load_header before load_setup") has moved that configuration section part to qemu_loadvm_state_header() which now happens before qemu_loadvm_state_setup(). Remove the cleanup calls that are now misplaced. CC: qemu-stable@nongnu.org Fixes: 9e14b84908 ("migration/savevm: load_header before load_setup") Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/savevm.c | 2 -- 1 file changed, 2 deletions(-)