Re: [PULL 02/45] migration: push Error **errp into vmstate_load_state()

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 21, 2025 at 06:22:55PM +0100, Peter Maydell wrote:
> On Tue, 21 Oct 2025 at 18:05, Peter Xu <peterx@redhat.com> wrote:
> >
> > On Tue, Oct 21, 2025 at 05:49:51PM +0100, Peter Maydell wrote:
> > > I suppose so, but that seems like something in practice
> > > we make a lot of use of. It's really handy to be able to
> > > say "this is how you obtain the integer you wanted to put
> > > in the migration stream" -- we do a fair amount of that
> > > in target/arm/machine.c for instance. But those don't
> > > need to return a failure, I suppose.
> >
> > That's exactly what pre_save() / post_load() should do, IMHO.
> >
> > Taking example of the arm's case here:
> >
> > static int put_fpscr(QEMUFile *f, void *opaque, size_t size,
> >                      const VMStateField *field, JSONWriter *vmdesc)
> > {
> >     ARMCPU *cpu = opaque;
> >     CPUARMState *env = &cpu->env;
> >     uint32_t fpscr = vfp_fpcr_fpsr_needed(opaque) ? 0 : vfp_get_fpscr(env);
> >
> >     qemu_put_be32(f, fpscr);
> >     return 0;
> > }
> >
> > The .pre_save() should be exactly the logic that generalize whatever we
> > have had in QEMU's structs into a pure uint32_t, put that into a temp
> > u32. Then migration should be able to transfer that using whatever way it
> > prefers.  When using qemufile API, it should use vmstate_info_uint32 so as
> > to not open-code qemu_put_be32().
> 
> I think this is very ugly, because it means that the device state
> struct ends up with a pile of extra fields whose only purpose
> is to be temporarily used during migration. Having a function
> which does the "figure out the value at the point where we want it"
> is a much nicer API because it keeps the migration related
> complication in the migration code, and keeps it out of the
> data structures that all the rest of the code has to work with.

It'll be a temporary struct, can be allocated only in a pre_save() and
freed in a post_save(), for example.  And IIUC most of the devices do not
need it when there're already good representation of the data for migration
within the device object.  AFAICT, the latter is the common case.

> 
> > Now, we have a major issue with qemufile and all the APIs that bound to it
> > when we want to move the IO layers from qemufile to iochannels, exactly
> > because we lack such abstraction layer, where we mixed up "this is how you
> > obtain the integer" and "send this integer to the wire" operations in one
> > API.
> 
> Yeah, this is unfortunate. But I don't think the solution is
> to require a lot of extra device state struct fields to carry
> temporary data for migration. It would be better to have an
> API similar to the existing get/put functions but which
> passes the data back to the caller instead of calling
> qemu_put_be32() itself.

Yes.  I feel like what we said do not conflict with each other.

What you mentioned looks like a per-field version of VMSD's pre/post hooks,
so AFAIU get() is almost a post_load(), and put() is almost a pre_save(),
only that it will be per-vmsd-field, rather than per-vmsd.

The important part is IO path separations, IMHO.

Thanks,

-- 
Peter Xu