On 2025/07/28 19:54, Arun Menon wrote:
+ error_free_or_abort(errp);
+ error_propagate(errp, local_err);
+ ret = ps_ret;
+ }
+ } else if (vmsd->post_save) {
+ int ps_ret = vmsd->post_save(opaque);
+ if (ps_ret < 0) {
+ ret = ps_ret;
+ }
}
return ret;
}
@@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const
VMStateDescription *vmsd,
ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
- if (vmsd->post_save) {
+ if (vmsd->post_save_errp) {
+ int ps_ret = vmsd->post_save_errp(opaque, &local_err);
+ if (!ret && ps_ret) {
+ ret = ps_ret;
+ error_propagate(errp, local_err);
+ } else if (ret && ps_ret) {
+ error_free_or_abort(errp);
+ error_propagate(errp, local_err);
+ ret = ps_ret;
+ }
Simpler:
if (ps_ret) {
if (ret) {
error_free_or_abort(errp);
}
ret = ps_ret;
error_propagate(errp, local_err);
}
Will do.
+ } else if (vmsd->post_save) {
int ps_ret = vmsd->post_save(opaque);
if (!ret && ps_ret) {
ret = ps_ret;
When there is a preceding error, this code still returns it and dismisses
the post_save() error although the other part of this function is changed to
propagate the error of post-save unconditionally. Please keep them
consistent.
I do feel that in the new implementation (post_save_errp), we should be
dismissing
the preceeding error and propagating the new error from post_save().
Because, that way, it makes sense to run the post_save_errp() part even after
encountering
an error in the preceeding section (vmstate_subsection_save).
What to propagate to the caller shouldn't matter when running the
post_save_errp() because the behavior is for the caller and not for the
internal implementation of this function.
There are two requirements for this function:
1. The proceeding error should be passed to the caller
2. The proceeding error should not prevent calling post_save_errp()
Let's assume whether the proceeding error is dismissed or not affects
both post_save_errp() and the caller. Under this assumption,
- if the proceeding error is dismissed, it will violate condition 1.
- if the proceeding error is not dismissed, it will violate condition 2.
So it is more reasonable to think that we are defining different
handling methods for the caller and post_save_errp().
If error propagation for the caller still has an implication on the
condition to run post_save_errp(), it means the meanings of the
proceeding error for post_save_errp() and the caller are "different but
somehow correlated". It sounds convoluted and I don't have an idea of
such correlation.
I don't think there is a common answer for what error to propagate when
there are several errors, so I see three options:
- propagate whatever error easier to do so
- somehow combine errors
(languages that support exceptions often do this)
- make sure that no error will happen when there is a proceeding error
by having two callbacks:
- One callback that is called only when there is no proceeding error
and can raise an error
- Another that is always called but cannot raise an error
Not sure if I should change the old impl post_save to match post_save_errp,
Or should I match the new impl with the old one.
The reasoning you provided applies for post_save() too, so I think they
should have a consistent behavior if you stick to it.
That change is also better to be split into its own patch; by splitting
it, you can ensure that, if this series causes a regression for TPM,
bisect will tell what led to the regression: the return value
propagation change or the addition of errp.
Regards,
Akihiko Odaki
