Hi Akihiko, Thanks for the review. On Fri, Aug 01, 2025 at 04:35:34PM +0900, Akihiko Odaki wrote: > On 2025/07/31 22:21, Arun Menon wrote: > > Currently post_save hook is called without checking its return. If post_save > > fails, we need to set an error and propagate it to the caller. > > > > Since post_save hook is called regardless of whether there is a preceeding > > error, > > it is possible that we have 2 distict errors, one from the preceeding > > function > > call, and the other from the post_save call. > > > > Return the latest error to the caller. > > This needs to be explained better. This patch makes two changes on the > behavior when there are two errors: > > 1) Proceeding errors were propagated before, but they are now > dismissed. > 2) post_error() errors were dismissed before, but they are now > propagated. > > This message doesn't mention 1) at all. It does say 2) is necessary, but > does not explain why.
Please correct me if I am wrong, does the following look ok? Currently post_save() hook is called without checking its return. post_save() hooks, generally does the cleanup, and that is the reason why we have been dismissing its failure. We want to consider - the saving of the device state (save or subsection save) and - running the cleanup after as an atomic operation. And that is why, post_save is called regardless of whether there is a preceeding error. This means that, it is possible that we have 2 distict errors, one from the preceeding function and the other from the post_save() function. This patch makes two changes on the behavior when there are two errors: 1) Preceeding errors were propagated before, but they are now dismissed if there is a new post_save() error. 2) post_save() errors were dismissed before, but they are now propagated. We intend to extend the error propagation feature (saving erros in Error object and return to the caller) to the post/pre save/load hooks. Therefore, it is important for the user to know the exact reason of failure in case any of these hooks fail. ==== I do not know much about the operations we do, or intend to do using the post_save() call. But my guess is that we are going to revert things or cleanup stuff, and if reverting/cleanup fails, then the user should be notified about that. > > > > > Signed-off-by: Arun Menon <[email protected]> > > --- > > migration/vmstate.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/migration/vmstate.c b/migration/vmstate.c > > index > > b725202bfcf69c3c81338f1f5479aa2ddc5db86f..25a819da069b982d4043f287b4562ea402d9eb0e > > 100644 > > --- a/migration/vmstate.c > > +++ b/migration/vmstate.c > > @@ -418,6 +418,7 @@ int vmstate_save_state_v(QEMUFile *f, const > > VMStateDescription *vmsd, > > void *opaque, JSONWriter *vmdesc, int > > version_id, Error **errp) > > { > > int ret = 0; > > + int ps_ret = 0; > > const VMStateField *field = vmsd->fields; > > trace_vmstate_save_state_top(vmsd->name); > > @@ -533,7 +534,14 @@ int vmstate_save_state_v(QEMUFile *f, const > > VMStateDescription *vmsd, > > error_setg(errp, "Save of field %s/%s failed", > > vmsd->name, field->name); > > if (vmsd->post_save) { > > - vmsd->post_save(opaque); > > + ps_ret = vmsd->post_save(opaque); > > + if (ps_ret) { > > + ret = ps_ret; > > + error_free_or_abort(errp); > > + error_setg(errp, > > + "post-save for %s failed, ret: > > '%d'", > > + vmsd->name, ret); > > + } > > } > > return ret; > > } > > @@ -561,10 +569,12 @@ int vmstate_save_state_v(QEMUFile *f, const > > VMStateDescription *vmsd, > > ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp); > > if (vmsd->post_save) { > > - int ps_ret = vmsd->post_save(opaque); > > - if (!ret && ps_ret) { > > + ps_ret = vmsd->post_save(opaque); > > + if (ps_ret) { > > ret = ps_ret; > > - error_setg(errp, "post-save failed: %s", vmsd->name); > > + error_free_or_abort(errp); > > + error_setg(errp, "post-save for %s failed, ret: '%d'", > > + vmsd->name, ret); > > } > > } > > return ret; > > > Regards, Arun
