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(). 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. 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(). > > > +} > > + > > 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.. > > > continue; > > } > > ret = vmstate_save(f, se, NULL, &local_err); > > @@ -2648,7 +2649,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t > > type, Error **errp) > > se->load_section_id = section_id; > > > > /* Validate if it is a device's state */ > > - if (xen_enabled() && se->is_ram) { > > + if (xen_enabled() && qemu_savevm_se_iterable(se)) { > > error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", > > idstr); > > return -EINVAL; > > } > -- Peter Xu
