[PATCH v3 0/1] migration: Check for duplicates on vmstate_register()

[PATCH v3 0/1] migration: Check for duplicates on vmstate_register()

[Juan Quintela]

Hi

on this v3:
- rebase on top of pull migration-20231024
- only one patch leaft.

Based-on: Message-ID: <20231024131305.87468-1-quintela@redhat.com>
          [PULL 00/39] Migration 20231024 patches

Please review.

Hi

on this v2:
- rebased on top of master (no conflicts)
- handled reviewed by
- improved documentation
- Changed the ppc hack to maintain backwards compatibility.

Please review.

[v1]
This series are based in a patch from Peter than check if a we try to
register the same device with the same instance_id more than once.  It
was not merged when he sent it because it broke "make check".  So I
fixed all devices to be able to merge it.

- I create vmstate_register_any(), its the same that
  vmstate_register(VMSTATE_INSTANCE_ID_ANY)
- Later I check in vmstate_register() that they are not calling it
  with VMSTATE_INSTANCE_ID_ANY
- After that I change vmstate_register() to make sure that we don't
  include a duplicate.

And we get all the errors that I change in patches 3, 4, 5, 6, 7.
After those patches: make check works again.
And then I reviewed all the rest of vmstate_register() callers.

There are the cases where they pass a device_id that is generated
somehow, that ones are ok.

Then we have the ones that pass always 0.  This ones are only valid
when there is a maximum of one device instantiated for a given
machine.

- audio: you can choose more than one audio output.
- eeprom93xx: you can have more than one e100 card.

- vmware_vga: I am not completely sure here, it appears that you could
  have more than one.  Notice that VMSTATE_INSTANCE_ID_ANY will give
  us the value 0 if there is only one instance, so we are in no
  trouble.  We can drop it if people think that we can't have more
  than one vmware_vga.

- for the rest of the devices, I can't see any that can be
  instantiated more than once (testing it is easy, just starting the
  machine will make it fail).  Notice that again, for the same
  reasoning, we could change all the calls to _any().  And only left
  the vmstate_register(... 0 ...) calls for devices that we know that
  we only ever want one.

What needs to be done:

- icp/server: We need to rename the old icp server name.  Notice that
  I doubt that anyone is migrating this, but I need help from PPC
  experts.  As said in the commit message, it is "abusing" the interface:
  - it register a new device
  - it realizes that it is instantiting an old beard
  - it unregister the new device
  - it registers the old device

- rest of devices:

  * pxa2xx devices: I can't see how you can create more than one
    device in a machine
  * acpi_build: I can't see how to create more than once.
  * replay: neither
  * cpu timers: created in vl.c
  * global_state: only once
  * s390 css: not a way that I can think
  * spapr: looks only one
  * or1ktimer: I can only see one
  * tsc*: I see only use in pxa2xx and one by board

- And now, another abuser:

vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,

tcet->liobn is an uint32_t, and instance_id is an int.  And it just happens that is value is < VMSTATE_INSTANCE_ID_ANY.

Please, review.

Juan Quintela (1):
  migration: vmstate_register() check that instance_id is valid

 include/migration/vmstate.h | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.41.0

[PATCH v3 1/1] migration: vmstate_register() check that instance_id is valid

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/vmstate.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9821918631..c48cd8bb68 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -28,6 +28,7 @@
 #define QEMU_VMSTATE_H
 
 #include "hw/vmstate-if.h"
+#include "qemu/error-report.h"
 
 typedef struct VMStateInfo VMStateInfo;
 typedef struct VMStateField VMStateField;
@@ -1226,6 +1227,11 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque)
 {
+    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
+        error_report("vmstate_register: Invalid device: %s instance_id: %d",
+                     vmsd->name, instance_id);
+        return -1;
+    }
     return vmstate_register_with_alias_id(obj, instance_id, vmsd,
                                           opaque, -1, 0, NULL);
 }
-- 
2.41.0

Re: [PATCH v3 1/1] migration: vmstate_register() check that instance_id is valid

[Peter Xu]

On Tue, Oct 24, 2023 at 05:03:36PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/vmstate.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9821918631..c48cd8bb68 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -28,6 +28,7 @@
>  #define QEMU_VMSTATE_H
>  
>  #include "hw/vmstate-if.h"
> +#include "qemu/error-report.h"
>  
>  typedef struct VMStateInfo VMStateInfo;
>  typedef struct VMStateField VMStateField;
> @@ -1226,6 +1227,11 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque)
>  {
> +    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
> +        error_report("vmstate_register: Invalid device: %s instance_id: %d",
> +                     vmsd->name, instance_id);
> +        return -1;
> +    }
>      return vmstate_register_with_alias_id(obj, instance_id, vmsd,
>                                            opaque, -1, 0, NULL);
>  }

Juan, could you remind me what's the benefit of failing it like that?

IIUC you want to suggest using vmstate_register_any(), but I think it's all
fine to do vmstate_register(VMSTATE_INSTANCE_ID_ANY)?  You didn't have a
commit message, so I am guessing..

Even if that is wanted, the current error message can be confusing to a
developer adding a new vmstate_register(VMSTATE_INSTANCE_ID_ANY) call.
Maybe directly suggest vmstate_register_any() in the error message?  But
again, I don't see a benefit, vmstate_register(VMSTATE_INSTANCE_ID_ANY)
should still work if without this patch?  Where did I miss?

Thanks,

-- 
Peter Xu

Re: [PATCH v3 1/1] migration: vmstate_register() check that instance_id is valid

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Oct 24, 2023 at 05:03:36PM +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  include/migration/vmstate.h | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 9821918631..c48cd8bb68 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -28,6 +28,7 @@
>>  #define QEMU_VMSTATE_H
>>  
>>  #include "hw/vmstate-if.h"
>> +#include "qemu/error-report.h"
>>  
>>  typedef struct VMStateInfo VMStateInfo;
>>  typedef struct VMStateField VMStateField;
>> @@ -1226,6 +1227,11 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
>>                                     const VMStateDescription *vmsd,
>>                                     void *opaque)
>>  {
>> +    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
>> +        error_report("vmstate_register: Invalid device: %s instance_id: %d",
>> +                     vmsd->name, instance_id);
>> +        return -1;
>> +    }
>>      return vmstate_register_with_alias_id(obj, instance_id, vmsd,
>>                                            opaque, -1, 0, NULL);
>>  }
>
> Juan, could you remind me what's the benefit of failing it like that?


> IIUC you want to suggest using vmstate_register_any(), but I think it's all
> fine to do vmstate_register(VMSTATE_INSTANCE_ID_ANY)?  You didn't have a
> commit message, so I am guessing..

This is v3.  v1 and v2 had much more messages, so I thought this was not
necessary.

We had lots of places that had vmstate_register(..., 0, ...) where it
should have s/0/VMSTATE_INSTANCE_ID_ANY/

The idea here is that we use vmstate_register_any(...) when we don't
care about the number and we know there is only to be one device.

On my tree, I started with the test:

    if (instance_id < 0) {
        error_report("vmstate_register: Invalid device: %s instance_id: %d",
                     vmsd->name, instance_id);
        return -1;
    }

But then ppc abuses this interface and passes an uint32_t where it
should be an int, so I have to check only for that specific value.

> Even if that is wanted, the current error message can be confusing to a
> developer adding a new vmstate_register(VMSTATE_INSTANCE_ID_ANY) call.
> Maybe directly suggest vmstate_register_any() in the error message?  But
> again, I don't see a benefit, vmstate_register(VMSTATE_INSTANCE_ID_ANY)
> should still work if without this patch?  Where did I miss?

You are right, using the other interface.

Initial version on this series, I split vmstate_register() into:
- vmstate_register_any()
- vmstate_register_id()  /* the difference with vmstate_register() was
                            just this test */

After auditing all the callers, I decided that using
vmstate_register_id() didn't brough we a lot, so I just dropped that
patches but left the test.

Forcing to use vmstate_register_any() makes easier to grep for the
places that try to use the vmstate_register(), but perhaps that is not
enough convenient.

Later, Juan.

Re: [PATCH v3 1/1] migration: vmstate_register() check that instance_id is valid

[Peter Xu]

On Tue, Oct 24, 2023 at 06:08:40PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Tue, Oct 24, 2023 at 05:03:36PM +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  include/migration/vmstate.h | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 9821918631..c48cd8bb68 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -28,6 +28,7 @@
> >>  #define QEMU_VMSTATE_H
> >>  
> >>  #include "hw/vmstate-if.h"
> >> +#include "qemu/error-report.h"
> >>  
> >>  typedef struct VMStateInfo VMStateInfo;
> >>  typedef struct VMStateField VMStateField;
> >> @@ -1226,6 +1227,11 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
> >>                                     const VMStateDescription *vmsd,
> >>                                     void *opaque)
> >>  {
> >> +    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
> >> +        error_report("vmstate_register: Invalid device: %s instance_id: %d",
> >> +                     vmsd->name, instance_id);
> >> +        return -1;
> >> +    }
> >>      return vmstate_register_with_alias_id(obj, instance_id, vmsd,
> >>                                            opaque, -1, 0, NULL);
> >>  }
> >
> > Juan, could you remind me what's the benefit of failing it like that?
> 
> 
> > IIUC you want to suggest using vmstate_register_any(), but I think it's all
> > fine to do vmstate_register(VMSTATE_INSTANCE_ID_ANY)?  You didn't have a
> > commit message, so I am guessing..
> 
> This is v3.  v1 and v2 had much more messages, so I thought this was not
> necessary.
> 
> We had lots of places that had vmstate_register(..., 0, ...) where it
> should have s/0/VMSTATE_INSTANCE_ID_ANY/
> 
> The idea here is that we use vmstate_register_any(...) when we don't
> care about the number and we know there is only to be one device.
> 
> On my tree, I started with the test:
> 
>     if (instance_id < 0) {
>         error_report("vmstate_register: Invalid device: %s instance_id: %d",
>                      vmsd->name, instance_id);
>         return -1;
>     }
> 
> But then ppc abuses this interface and passes an uint32_t where it
> should be an int, so I have to check only for that specific value.
> 
> > Even if that is wanted, the current error message can be confusing to a
> > developer adding a new vmstate_register(VMSTATE_INSTANCE_ID_ANY) call.
> > Maybe directly suggest vmstate_register_any() in the error message?  But
> > again, I don't see a benefit, vmstate_register(VMSTATE_INSTANCE_ID_ANY)
> > should still work if without this patch?  Where did I miss?
> 
> You are right, using the other interface.
> 
> Initial version on this series, I split vmstate_register() into:
> - vmstate_register_any()
> - vmstate_register_id()  /* the difference with vmstate_register() was
>                             just this test */
> 
> After auditing all the callers, I decided that using
> vmstate_register_id() didn't brough we a lot, so I just dropped that
> patches but left the test.
> 
> Forcing to use vmstate_register_any() makes easier to grep for the
> places that try to use the vmstate_register(), but perhaps that is not
> enough convenient.

IMHO if we have the dup check in vmstate_register_with_alias_id(),
instance_id isn't a problem anymore; no abuse should happen without failing
that already.

Personally I tend to just drop this one.  If to keep it, maybe change the
error message to suggest the right one, then let it still proceed?  Because
it still works.  The error will only be a hint but help not so much, IMHO.
What do you think?

Thanks,

-- 
Peter Xu

Re: [PATCH v3 1/1] migration: vmstate_register() check that instance_id is valid

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:

>> > IIUC you want to suggest using vmstate_register_any(), but I think it's all
>> > fine to do vmstate_register(VMSTATE_INSTANCE_ID_ANY)?  You didn't have a
>> > commit message, so I am guessing..
>> 
>> This is v3.  v1 and v2 had much more messages, so I thought this was not
>> necessary.
>> 
>> We had lots of places that had vmstate_register(..., 0, ...) where it
>> should have s/0/VMSTATE_INSTANCE_ID_ANY/
>> 
>> The idea here is that we use vmstate_register_any(...) when we don't
>> care about the number and we know there is only to be one device.
>> 
>> On my tree, I started with the test:
>> 
>>     if (instance_id < 0) {
>>         error_report("vmstate_register: Invalid device: %s instance_id: %d",
>>                      vmsd->name, instance_id);
>>         return -1;
>>     }
>> 
>> But then ppc abuses this interface and passes an uint32_t where it
>> should be an int, so I have to check only for that specific value.
>> 
>> > Even if that is wanted, the current error message can be confusing to a
>> > developer adding a new vmstate_register(VMSTATE_INSTANCE_ID_ANY) call.
>> > Maybe directly suggest vmstate_register_any() in the error message?  But
>> > again, I don't see a benefit, vmstate_register(VMSTATE_INSTANCE_ID_ANY)
>> > should still work if without this patch?  Where did I miss?
>> 
>> You are right, using the other interface.
>> 
>> Initial version on this series, I split vmstate_register() into:
>> - vmstate_register_any()
>> - vmstate_register_id()  /* the difference with vmstate_register() was
>>                             just this test */
>> 
>> After auditing all the callers, I decided that using
>> vmstate_register_id() didn't brough we a lot, so I just dropped that
>> patches but left the test.
>> 
>> Forcing to use vmstate_register_any() makes easier to grep for the
>> places that try to use the vmstate_register(), but perhaps that is not
>> enough convenient.
>
> IMHO if we have the dup check in vmstate_register_with_alias_id(),
> instance_id isn't a problem anymore; no abuse should happen without failing
> that already.
>
> Personally I tend to just drop this one.  If to keep it, maybe change the
> error message to suggest the right one, then let it still proceed?  Because
> it still works.  The error will only be a hint but help not so much, IMHO.
> What do you think?

I would preffer it, but not enough to fight for it, so dropping it.

Thanks, Juan.