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