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

[Juan Quintela <quintela@redhat.com>]

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.