It's neither accurate nor necessary. Use a proper helper to detect if it's
an iterable savevm state entry instead.
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;
+}
+
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)) {
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;
}
--
2.50.1Peter 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?
>
> 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?
> +}
> +
> 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.
> 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;
> }
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().
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
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>
© 2016 - 2026 Red Hat, Inc.