On Fri, Oct 24, 2025 at 03:20:58AM +0530, Arun Menon wrote: > 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.
Yes, dumping the errors should be better. The other thing is even if virtio_gpu_load() ignored vmstate_load_state()'s retval, I think it should.. If you agree you can also fix that together. In general, likely we should never use error_abort in vmstate_load_state(). Thanks, -- Peter Xu
