| Message ID | 20240304122844.1888308-5-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Improve error reporting | expand |
Cédric Le Goater <clg@redhat.com> writes: > This will prepare ground for futur changes adding an Error** argument > to the save_setup() handler. We need to make sure that on failure, > ram_save_setup() sets a new error. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > migration/ram.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 45a00b45edd429ef0568adce09b7459883e00167..dbd04d8ee2167881007c836a6bbc79c1aced72d0 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -2937,12 +2937,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > int ret; > > if (compress_threads_save_setup()) { > + error_report("%s: failed to start compress threads", __func__); > return -1; > } > > /* migration has already setup the bitmap, reuse it. */ > if (!migration_in_colo_state()) { > if (ram_init_all(rsp) != 0) { > + error_report("%s: failed to setup RAM for migration", __func__); > compress_threads_save_cleanup(); > return -1; > } > @@ -2969,12 +2971,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > > ret = rdma_registration_start(f, RAM_CONTROL_SETUP); > if (ret < 0) { > + error_report("%s: failed to start RDMA registration", __func__); > qemu_file_set_error(f, ret); > return ret; > } > > ret = rdma_registration_stop(f, RAM_CONTROL_SETUP); > if (ret < 0) { > + error_report("%s: failed to stop RDMA registration", __func__); > qemu_file_set_error(f, ret); > return ret; > } > @@ -2986,6 +2990,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > ret = multifd_send_sync_main(); > bql_lock(); > if (ret < 0) { > + error_report("%s: multifd synchronization failed", __func__); > return ret; > } > > @@ -2994,7 +2999,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > } > > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > - return qemu_fflush(f); > + ret = qemu_fflush(f); > + if (ret) { > + error_report("%s failed : %s", __func__, strerror(ret)); Here it should be -ret The qemu_fflush function returns the QEMUFile error (f->last_error) and that is expected to be a -errno value. I started making sure all callers of qemu_file_set_error() use a negative value, but got lost in the vmstate code and never finished: https://lore.kernel.org/r/20230706195201.18595-1-farosas@suse.de
On Mon, 4 Mar 2024 at 18:01, Cédric Le Goater <clg@redhat.com> wrote: > This will prepare ground for futur changes adding an Error** argument * futur -> future > + ret = qemu_fflush(f); > + if (ret) { * if (ret) -> if (ret < 0) Thank you. --- - Prasad
On 3/4/24 22:30, Fabiano Rosas wrote: > Cédric Le Goater <clg@redhat.com> writes: > >> This will prepare ground for futur changes adding an Error** argument >> to the save_setup() handler. We need to make sure that on failure, >> ram_save_setup() sets a new error. >> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> migration/ram.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 45a00b45edd429ef0568adce09b7459883e00167..dbd04d8ee2167881007c836a6bbc79c1aced72d0 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -2937,12 +2937,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> int ret; >> >> if (compress_threads_save_setup()) { >> + error_report("%s: failed to start compress threads", __func__); >> return -1; >> } >> >> /* migration has already setup the bitmap, reuse it. */ >> if (!migration_in_colo_state()) { >> if (ram_init_all(rsp) != 0) { >> + error_report("%s: failed to setup RAM for migration", __func__); >> compress_threads_save_cleanup(); >> return -1; >> } >> @@ -2969,12 +2971,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> >> ret = rdma_registration_start(f, RAM_CONTROL_SETUP); >> if (ret < 0) { >> + error_report("%s: failed to start RDMA registration", __func__); >> qemu_file_set_error(f, ret); >> return ret; >> } >> >> ret = rdma_registration_stop(f, RAM_CONTROL_SETUP); >> if (ret < 0) { >> + error_report("%s: failed to stop RDMA registration", __func__); >> qemu_file_set_error(f, ret); >> return ret; >> } >> @@ -2986,6 +2990,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> ret = multifd_send_sync_main(); >> bql_lock(); >> if (ret < 0) { >> + error_report("%s: multifd synchronization failed", __func__); >> return ret; >> } >> >> @@ -2994,7 +2999,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> } >> >> qemu_put_be64(f, RAM_SAVE_FLAG_EOS); >> - return qemu_fflush(f); >> + ret = qemu_fflush(f); >> + if (ret) { >> + error_report("%s failed : %s", __func__, strerror(ret)); > > Here it should be -ret > > The qemu_fflush function returns the QEMUFile error (f->last_error) and > that is expected to be a -errno value. oh yes. I should have know that since qemu_file_set_error() takes a -errno. > I started making sure all callers of qemu_file_set_error() use a > negative value, but got lost in the vmstate code and never finished: > > https://lore.kernel.org/r/20230706195201.18595-1-farosas@suse.de Thanks for the pointer. Thanks, C.
On 3/5/24 06:29, Prasad Pandit wrote: > On Mon, 4 Mar 2024 at 18:01, Cédric Le Goater <clg@redhat.com> wrote: >> This will prepare ground for futur changes adding an Error** argument > > * futur -> future Fixed in all first 5 patches. > >> + ret = qemu_fflush(f); >> + if (ret) { > > * if (ret) -> if (ret < 0) yep. Thanks, C.
diff --git a/migration/ram.c b/migration/ram.c index 45a00b45edd429ef0568adce09b7459883e00167..dbd04d8ee2167881007c836a6bbc79c1aced72d0 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -2937,12 +2937,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) int ret; if (compress_threads_save_setup()) { + error_report("%s: failed to start compress threads", __func__); return -1; } /* migration has already setup the bitmap, reuse it. */ if (!migration_in_colo_state()) { if (ram_init_all(rsp) != 0) { + error_report("%s: failed to setup RAM for migration", __func__); compress_threads_save_cleanup(); return -1; } @@ -2969,12 +2971,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) ret = rdma_registration_start(f, RAM_CONTROL_SETUP); if (ret < 0) { + error_report("%s: failed to start RDMA registration", __func__); qemu_file_set_error(f, ret); return ret; } ret = rdma_registration_stop(f, RAM_CONTROL_SETUP); if (ret < 0) { + error_report("%s: failed to stop RDMA registration", __func__); qemu_file_set_error(f, ret); return ret; } @@ -2986,6 +2990,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) ret = multifd_send_sync_main(); bql_lock(); if (ret < 0) { + error_report("%s: multifd synchronization failed", __func__); return ret; } @@ -2994,7 +2999,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque) } qemu_put_be64(f, RAM_SAVE_FLAG_EOS); - return qemu_fflush(f); + ret = qemu_fflush(f); + if (ret) { + error_report("%s failed : %s", __func__, strerror(ret)); + } + return ret; } /**
This will prepare ground for futur changes adding an Error** argument to the save_setup() handler. We need to make sure that on failure, ram_save_setup() sets a new error. Signed-off-by: Cédric Le Goater <clg@redhat.com> --- migration/ram.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)