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