Peter Xu <[email protected]> writes:
> On Fri, Jan 23, 2026 at 10:47:10AM -0300, Fabiano Rosas wrote:
>> Peter Xu <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 <[email protected]>
