Hi Peter, On Tue, Oct 21, 2025 at 12:16:53PM -0400, Peter Xu wrote: > On Tue, Oct 21, 2025 at 04:43:52PM +0100, Peter Maydell wrote: > > On Fri, 3 Oct 2025 at 16:39, Peter Xu <[email protected]> wrote: > > > > > > From: Arun Menon <[email protected]> > > > > > > This is an incremental step in converting vmstate loading > > > code to report error via Error objects instead of directly > > > printing it to console/monitor. > > > It is ensured that vmstate_load_state() must report an error > > > in errp, in case of failure. > > > > > > The errors are temporarily reported using error_report_err(). > > > This is removed in the subsequent patches in this series, > > > when we are actually able to propagate the error to the calling > > > function using errp. Whereas, if we want the function to exit on > > > error, then error_fatal is passed. > > > > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > > index de35902213..e61585aa61 100644 > > > --- a/hw/display/virtio-gpu.c > > > +++ b/hw/display/virtio-gpu.c > > > @@ -1347,7 +1347,7 @@ static int virtio_gpu_load(QEMUFile *f, void > > > *opaque, size_t size, > > > } > > > > > > /* load & apply scanout state */ > > > - vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1); > > > + vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, > > > &error_fatal); > > > > This is in a migration VMState .get function -- shouldn't we > > be passing failure up to the caller, rather than exiting > > with error_fatal here ? > > > > The commit message says some of this is fixed in subsequent > > patches, but as of today this is still the code in git. > > The other callsites which pass error_fatal to vmstate_load_state() > > also look wrong: > > > > hw/s390x/virtio-ccw.c: return vmstate_load_state(f, > > &vmstate_virtio_ccw_dev, dev, 1, &error_fatal); > > hw/virtio/virtio-mmio.c: return vmstate_load_state(f, > > &vmstate_virtio_mmio, proxy, 1, &error_fatal); > > hw/virtio/virtio-pci.c: return vmstate_load_state(f, > > &vmstate_virtio_pci, proxy, 1, &error_fatal); > > > > as they are written to return an error value that they'll > > never see because of the use of error_fatal here. > > Indeed a fair question to ask. > > > > > Do you have plans for further cleanup/extension of the > > use of Error here that would let these functions pass > > the Error back up the chain ? > > It would be non-trivial though as we'll need to change VMStateInfo.get() > API and that'll be another lot of churns. > > Arun, should we pass NULL for above three occurances, so that we will still > not brute force quit when error happens? Do you want to send a patch?
Sorry, I missed the email. I am wondering if we should pass an Error *err, and in case of failure, warn_report_err(err) and return a negative integer. There is no return value check after vmstate_load_state() or vmstate_save_state() calls. Previosuly, if something failed during vmstate_load_state(), the function used to report error using error_report(). To be consistent we might have to keep reporting. This is not possible if we pass NULL. Please guide. Thank you. > > -- > Peter Xu > Regards, Arun Menon
