| Message ID | 20240306133441.2351700-12-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Improve error reporting | expand |
On 06.03.24 16:34, Cédric Le Goater wrote: > The purpose is to record a potential error in the migration stream if > qemu_savevm_state_setup() fails. Most of the current .save_setup() > handlers can be modified to use the Error argument instead of managing > their own and calling locally error_report(). > > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> > Cc: Halil Pasic <pasic@linux.ibm.com> > Cc: Thomas Huth <thuth@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Cc: John Snow <jsnow@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Peter Xu <peterx@redhat.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Cédric Le Goater <clg@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Still, if you resend, please add error_prepend in the case below: > diff --git a/migration/savevm.c b/migration/savevm.c > index 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp) > } > save_section_header(f, se, QEMU_VM_SECTION_START); > > - ret = se->ops->save_setup(f, se->opaque); > + ret = se->ops->save_setup(f, se->opaque, errp); > save_section_footer(f, se); > if (ret < 0) { > - error_setg(errp, "failed to setup SaveStateEntry with id(name): " > - "%d(%s): %d", se->section_id, se->idstr, ret); You drop a good bit of information, let's use error_prepend to save it. > qemu_file_set_error(f, ret); > break; Not about this patch: Better do explicit "return ret" instead of this "break" (and one more break above in that loop): 1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more "cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto fail;". 2. "break" make me think, that there may be more logic after it, which will probably fail, and I should be careful, as errp is already set (and second attempt to set it will crash). Again, "goto fail;" is better, as I don't expect more failures when see it.
On 3/7/24 10:53, Vladimir Sementsov-Ogievskiy wrote: > On 06.03.24 16:34, Cédric Le Goater wrote: >> The purpose is to record a potential error in the migration stream if >> qemu_savevm_state_setup() fails. Most of the current .save_setup() >> handlers can be modified to use the Error argument instead of managing >> their own and calling locally error_report(). >> >> Cc: Nicholas Piggin <npiggin@gmail.com> >> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> >> Cc: Halil Pasic <pasic@linux.ibm.com> >> Cc: Thomas Huth <thuth@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> Cc: John Snow <jsnow@redhat.com> >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Peter Xu <peterx@redhat.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > Still, if you resend, please add error_prepend in the case below: > >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp) >> } >> save_section_header(f, se, QEMU_VM_SECTION_START); >> - ret = se->ops->save_setup(f, se->opaque); >> + ret = se->ops->save_setup(f, se->opaque, errp); >> save_section_footer(f, se); >> if (ret < 0) { >> - error_setg(errp, "failed to setup SaveStateEntry with id(name): " >> - "%d(%s): %d", se->section_id, se->idstr, ret); > > You drop a good bit of information, let's use error_prepend to save it. I kind of agree but the call stack is quite deep and the callees also use error_prepend. The error becomes quite long. Here's an example of what we get today : (qemu) migrate -d tcp:10.8.3.15:1234 (qemu) (qemu) qemu-system-x86_64: vfio: Could not start dirty page tracking - 0000:b1:00.2: Failed to start DMA logging: Invalid argument If the subsystems implementing a .save_setup() handler use a component identifier, the failure should be fairly easy to identify. What's the best practice for such cases ? Can we use multiline errors maybe ? Less practical for grep though. May be a verbose error mode would help getting more information ? Anyhow, I can add a new trace event for "failed to setup SaveStateEntry ... " or use error_prepend() as you suggested. Let's see what the others have to say. > >> qemu_file_set_error(f, ret); >> break; > > Not about this patch: > > Better do explicit "return ret" instead of this "break" (and one more break above in that loop): > > 1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more "cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto fail;". > 2. "break" make me think, that there may be more logic after it, which will probably fail, and I should be careful, as errp is already set (and second attempt to set it will crash). Again, "goto fail;" is better, as I don't expect more failures when see it. Sure. If I respin, I can drop the break and simply return. Although, I would be glad to have most of this series merged in QEMU 9.0. So, unless there is something major, I will keep that for followups. Thanks for the review, C.
On 07.03.24 13:31, Cédric Le Goater wrote: > On 3/7/24 10:53, Vladimir Sementsov-Ogievskiy wrote: >> On 06.03.24 16:34, Cédric Le Goater wrote: >>> The purpose is to record a potential error in the migration stream if >>> qemu_savevm_state_setup() fails. Most of the current .save_setup() >>> handlers can be modified to use the Error argument instead of managing >>> their own and calling locally error_report(). >>> >>> Cc: Nicholas Piggin <npiggin@gmail.com> >>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> >>> Cc: Halil Pasic <pasic@linux.ibm.com> >>> Cc: Thomas Huth <thuth@redhat.com> >>> Cc: Eric Blake <eblake@redhat.com> >>> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> Cc: John Snow <jsnow@redhat.com> >>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Reviewed-by: Peter Xu <peterx@redhat.com> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> >> Still, if you resend, please add error_prepend in the case below: >> >>> diff --git a/migration/savevm.c b/migration/savevm.c >>> index 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013 100644 >>> --- a/migration/savevm.c >>> +++ b/migration/savevm.c >>> @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp) >>> } >>> save_section_header(f, se, QEMU_VM_SECTION_START); >>> - ret = se->ops->save_setup(f, se->opaque); >>> + ret = se->ops->save_setup(f, se->opaque, errp); >>> save_section_footer(f, se); >>> if (ret < 0) { >>> - error_setg(errp, "failed to setup SaveStateEntry with id(name): " >>> - "%d(%s): %d", se->section_id, se->idstr, ret); >> >> You drop a good bit of information, let's use error_prepend to save it. > > I kind of agree but the call stack is quite deep and the callees also use > error_prepend. The error becomes quite long. Here's an example of what we > get today : > > (qemu) migrate -d tcp:10.8.3.15:1234 > (qemu) > (qemu) qemu-system-x86_64: vfio: Could not start dirty page tracking - 0000:b1:00.2: Failed to start DMA logging: Invalid argument > > If the subsystems implementing a .save_setup() handler use a component > identifier, the failure should be fairly easy to identify. > > What's the best practice for such cases ? > > Can we use multiline errors maybe ? Less practical for grep though. > > May be a verbose error mode would help getting more information ? > > Anyhow, I can add a new trace event for "failed to setup SaveStateEntry ... " > or use error_prepend() as you suggested. > > Let's see what the others have to say. > > >> >>> qemu_file_set_error(f, ret); >>> break; >> >> Not about this patch: >> >> Better do explicit "return ret" instead of this "break" (and one more break above in that loop): >> >> 1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more "cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto fail;". >> 2. "break" make me think, that there may be more logic after it, which will probably fail, and I should be careful, as errp is already set (and second attempt to set it will crash). Again, "goto fail;" is better, as I don't expect more failures when see it. > > Sure. If I respin, I can drop the break and simply return. If so, you should also make simple return instead of one another break in same loop. And drop "if (ret < 0) { return ret }" after loop. > Although, > I would be glad to have most of this series merged in QEMU 9.0. So, > unless there is something major, I will keep that for followups. > Agree > > Thanks for the review, > > C. > > > > > >
On Thu, Mar 07, 2024 at 02:39:31PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > I would be glad to have most of this series merged in QEMU 9.0. So, > > unless there is something major, I will keep that for followups. Unfortunately I found this series won't apply to master.. starting from "migration: Always report an error in ram_save_setup()". Perhaps forgot to pull before the repost? It'll also be nice if we can get an ACK for the s390 patch from a maintainer. Cedric, would you prefer a repost before this weekend, or we just wait for 9.1? IMHO we don't need to rush this in 9.0 if it's still partially done, so the latter option isn't that bad (I've already queued the initial four irrelevant of that). Thanks,
On Fri, Mar 08, 2024 at 03:11:04PM +0800, Peter Xu wrote: > On Thu, Mar 07, 2024 at 02:39:31PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > I would be glad to have most of this series merged in QEMU 9.0. So, > > > unless there is something major, I will keep that for followups. > > Unfortunately I found this series won't apply to master.. starting from > "migration: Always report an error in ram_save_setup()". Perhaps forgot to > pull before the repost? Scratch this. It's myself who forgot to pull... :-( It applies all fine. > > It'll also be nice if we can get an ACK for the s390 patch from a > maintainer. I'll ping on the patch.
diff --git a/include/migration/register.h b/include/migration/register.h index d7b70a8be68c9df47c7843bda7d430989d7ca384..64fc7c11036c82edd6d69513e56a0216d36c17aa 100644 --- a/include/migration/register.h +++ b/include/migration/register.h @@ -60,10 +60,11 @@ typedef struct SaveVMHandlers { * * @f: QEMUFile where to send the data * @opaque: data pointer passed to register_savevm_live() + * @errp: pointer to Error*, to store an error if it happens. * * Returns zero to indicate success and negative for error */ - int (*save_setup)(QEMUFile *f, void *opaque); + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); /** * @save_cleanup diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 55263f0815ed7671b32ea20b394ae71c82e616cb..045c024ffa76eacfc496bd486cb6cafbee2df73e 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = { } }; -static int htab_save_setup(QEMUFile *f, void *opaque) +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp) { SpaprMachineState *spapr = opaque; diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c index b743e8a2fee84c7374460ccea6df1cf447cda44b..bc04187b2b69226db80219da1a964a87428adc0c 100644 --- a/hw/s390x/s390-stattrib.c +++ b/hw/s390x/s390-stattrib.c @@ -168,19 +168,17 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id) return ret; } -static int cmma_save_setup(QEMUFile *f, void *opaque) +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp) { S390StAttribState *sas = S390_STATTRIB(opaque); S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas); - Error *local_err = NULL; int res; /* * Signal that we want to start a migration, thus needing PGSTE dirty * tracking. */ - res = sac->set_migrationmode(sas, true, &local_err); + res = sac->set_migrationmode(sas, true, errp); if (res) { - error_report_err(local_err); return res; } qemu_put_be64(f, STATTR_FLAG_EOS); diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 330b3a28548e32b0b3268072895bb5e4875766a2..3e893093ea6191fda35b7fdaddad5bff23e97a13 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp) return 0; } -static int vfio_save_setup(QEMUFile *f, void *opaque) +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp) { VFIODevice *vbasedev = opaque; VFIOMigration *migration = vbasedev->migration; @@ -392,8 +392,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) stop_copy_size); migration->data_buffer = g_try_malloc0(migration->data_buffer_size); if (!migration->data_buffer) { - error_report("%s: Failed to allocate migration data buffer", - vbasedev->name); + error_setg(errp, "%s: Failed to allocate migration data buffer", + vbasedev->name); return -ENOMEM; } @@ -403,8 +403,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY, VFIO_DEVICE_STATE_RUNNING); if (ret) { - error_report("%s: Failed to set new PRE_COPY state", - vbasedev->name); + error_setg(errp, "%s: Failed to set new PRE_COPY state", + vbasedev->name); return ret; } @@ -415,8 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) /* vfio_save_complete_precopy() will go to STOP_COPY */ break; default: - error_report("%s: Invalid device state %d", vbasedev->name, - migration->device_state); + error_setg(errp, "%s: Invalid device state %d", vbasedev->name, + migration->device_state); return -EINVAL; } } @@ -427,8 +427,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) ret = qemu_file_get_error(f); if (ret < 0) { - error_report("%s: save setup failed : %s", vbasedev->name, - strerror(-ret)); + error_setg_errno(errp, -ret, "%s: save setup failed", vbasedev->name); } return ret; diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 2708abf3d762de774ed294d3fdb8e56690d2974c..542a8c297b329abc30d1b3a205d29340fa59a961 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -1213,12 +1213,14 @@ fail: return ret; } -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque) +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp) { DBMSaveState *s = &((DBMState *)opaque)->save; SaveBitmapState *dbms = NULL; if (init_dirty_bitmap_migration(s) < 0) { + error_setg(errp, + "Failed to initialize dirty tracking bitmap for blocks"); return -1; } diff --git a/migration/block.c b/migration/block.c index aa65ec718c2875ad9d1c40c971035f14d8086a6e..86c1e0e5dd9bfcca33d711600359f12f16f99b9a 100644 --- a/migration/block.c +++ b/migration/block.c @@ -710,10 +710,9 @@ static void block_migration_cleanup(void *opaque) blk_mig_unlock(); } -static int block_save_setup(QEMUFile *f, void *opaque) +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp) { int ret; - Error *local_err = NULL; trace_migration_block_save("setup", block_mig_state.submitted, block_mig_state.transferred); @@ -721,25 +720,21 @@ static int block_save_setup(QEMUFile *f, void *opaque) warn_report("block migration is deprecated;" " use blockdev-mirror with NBD instead"); - ret = init_blk_migration(f, &local_err); + ret = init_blk_migration(f, errp); if (ret < 0) { - error_report_err(local_err); return ret; } /* start track dirty blocks */ ret = set_dirty_tracking(); if (ret) { - error_setg_errno(&local_err, -ret, - "Failed to start block dirty tracking"); - error_report_err(local_err); + error_setg_errno(errp, -ret, "Failed to start block dirty tracking"); return ret; } ret = flush_blks(f); if (ret) { - error_setg_errno(&local_err, -ret, "Flushing block failed"); - error_report_err(local_err); + error_setg_errno(errp, -ret, "Flushing block failed"); return ret; } blk_mig_reset_dirty_cursor(); diff --git a/migration/ram.c b/migration/ram.c index 3ac7f52a5f8e2c0d78a8cf150b3fa6611e12ffcc..52ad519b305532284003d78b93dd4a7186c767af 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3049,22 +3049,23 @@ static bool mapped_ram_read_header(QEMUFile *file, MappedRamHeader *header, * * @f: QEMUFile where to send the data * @opaque: RAMState pointer + * @errp: pointer to Error*, to store an error if it happens. */ -static int ram_save_setup(QEMUFile *f, void *opaque) +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) { RAMState **rsp = opaque; RAMBlock *block; int ret, max_hg_page_size; if (compress_threads_save_setup()) { - error_report("%s: failed to start compress threads", __func__); + error_setg(errp, "%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__); + error_setg(errp, "%s: failed to setup RAM for migration", __func__); compress_threads_save_cleanup(); return -1; } @@ -3101,14 +3102,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__); + error_setg(errp, "%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__); + error_setg(errp, "%s: failed to stop RDMA registration", __func__); qemu_file_set_error(f, ret); return ret; } @@ -3120,7 +3121,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__); + error_setg(errp, "%s: multifd synchronization failed", __func__); return ret; } @@ -3132,7 +3133,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) qemu_put_be64(f, RAM_SAVE_FLAG_EOS); ret = qemu_fflush(f); if (ret < 0) { - error_report("%s failed : %s", __func__, strerror(-ret)); + error_setg_errno(errp, -ret, "%s failed", __func__); } return ret; } diff --git a/migration/savevm.c b/migration/savevm.c index 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp) } save_section_header(f, se, QEMU_VM_SECTION_START); - ret = se->ops->save_setup(f, se->opaque); + ret = se->ops->save_setup(f, se->opaque, errp); save_section_footer(f, se); if (ret < 0) { - error_setg(errp, "failed to setup SaveStateEntry with id(name): " - "%d(%s): %d", se->section_id, se->idstr, ret); qemu_file_set_error(f, ret); break; }