Re: [PATCH 04/10] migration/savevm: Remove SaveStateEntry.is_ram

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jan 23, 2026 at 10:47:10AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > It's neither accurate nor necessary.  Use a proper helper to detect if it's
>> > an iterable savevm state entry instead.
>> 
>> Could you explain what exactly is_ram was supposed to mean?
>
> IIUC when introduced it was trying to capture the ram only, and that's also
> why I said it's inaccurate, because ram is not the only thing that can
> provide a save_setup() nowadays..
>
>> 
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/savevm.c | 15 ++++++++-------
>> >  1 file changed, 8 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/migration/savevm.c b/migration/savevm.c
>> > index 61e873d90c..f1cd8c913d 100644
>> > --- a/migration/savevm.c
>> > +++ b/migration/savevm.c
>> > @@ -249,7 +249,6 @@ typedef struct SaveStateEntry {
>> >      const VMStateDescription *vmsd;
>> >      void *opaque;
>> >      CompatEntry *compat;
>> > -    int is_ram;
>> >  } SaveStateEntry;
>> >  
>> >  typedef struct SaveState {
>> > @@ -816,10 +815,6 @@ int register_savevm_live(const char *idstr,
>> >      se->ops = ops;
>> >      se->opaque = opaque;
>> >      se->vmsd = NULL;
>> > -    /* if this is a live_savem then set is_ram */
>> > -    if (ops->save_setup != NULL) {
>> > -        se->is_ram = 1;
>> > -    }
>> >  
>> >      pstrcat(se->idstr, sizeof(se->idstr), idstr);
>> >  
>> > @@ -1866,6 +1861,12 @@ void qemu_savevm_live_state(QEMUFile *f)
>> >      qemu_put_byte(f, QEMU_VM_EOF);
>> >  }
>> >  
>> > +/* Is a save state entry iterable (e.g. RAM)? */
>> > +static bool qemu_savevm_se_iterable(SaveStateEntry *se)
>> > +{
>> > +    return se->ops && se->ops->save_setup;
>> 
>> So it could be iterable and not have .save_live_iterate?
>> 
>> Also, what's the deal with slirp? AIUI, it would be considered is_ram?
>
> I guess no. Either in term of when it's introduced, or in theory of how I
> understand this piece of code should work.. Because savevm_slirp_state
> doesn't provide save_setup().
>

Oops, I misread save_state vs. save_setup.

> Note that this patch didn't really change the fact on how it works, even if
> I _think_ this is broken..  I want to leave it for later, making sure this
> patch (while doing the cleanup) doesn't introduce functional changes.
>

That's always fine.

> So I expect things to break if e.g. COLO is used with VFIO devices (that
> supports migrations), where it also provides save_setup(), but actually
> COLO will not properly sync VFIO states.  Likely nobody is using it anyway.
>
> It's also the same to Xen's xen-save-devices-state QMP command.  It looks
> to me Xen is playing some "split savevm" work here, however I don't know
> whether it'll always workout these days when there're special devices that
> provide save_setup().
>

Hm, ok. Let's leave it then.

>> 
>> > +}
>> > +
>> >  int qemu_save_device_state(QEMUFile *f)
>> >  {
>> >      Error *local_err = NULL;
>> > @@ -1876,7 +1877,7 @@ int qemu_save_device_state(QEMUFile *f)
>> >      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>> >          int ret;
>> >  
>> > -        if (se->is_ram) {
>> > +        if (qemu_savevm_se_iterable(se)) {
>> 
>> Hmm, qemu_savevm_state_setup has:
>> 
>>     if (!se->ops || !se->ops->save_setup) {
>>             continue;
>>     }
>> 
>> therefore:
>> 
>>     if (!qemu_savevm_se_iterable(se)) {
>>             continue;
>>     }
>> 
>> Maybe it would be more practical to put these in a separate list already
>> while registering.
>
> This is a question about performance, my gut feeling is looping over with
> one queue would be fine for now, but even if not, it'll be further question
> to ask after above (on correctness..).
>
> So I plan to be focused on the cleanup part for now in this series..
>

Reviewed-by: Fabiano Rosas <farosas@suse.de>