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.