Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device

[Laszlo Ersek]

On 03/20/17 16:13, Laszlo Ersek wrote:
> On 03/20/17 15:16, Michael S. Tsirkin wrote:
>> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote:
>>> Multiple instances make no sense.
>>>
>>> Cc: "Michael S. Tsirkin" <address@hidden>
>>> Cc: Ben Warren <address@hidden>
>>> Cc: Igor Mammedov <address@hidden>
>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>
>> find_vmgenid_dev would be a better place for this.
>> This is where the single instance assumption comes from ATM.
> 
> object_resolve_path_type() -- used internally in find_vmgenid_dev() --
> returns NULL in either of two cases: there is no such device, or there
> are multiple devices. You can tell them apart by looking at the last
> parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that
> parameter.
> 
> By the time we are in the vmgenid_realize() function, at least one
> vmgenid device is guaranteed to exist (the one which we are realizing).
> Therefore, this patch could be simplified as:
> 
> if (find_vmgenid_dev() == NULL) {
>   error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE);
>   return;
> }
> 
> I found that confusing, and wanted to spell out "ambiguous" with the
> assert(). If you prefer the above simpler (but harder to understand)
> check, I can do that too.

Also, find_vmgenid_dev() only captures the single instance assumption,
it does not dictate the assumption. The assumption comes from the spec.

With the above in mind, what do you say about this patch? Do you want me
to call find_vmgenid_dev() in the realize function (which will require a
comment about object_resolve_path_type's behavior), or are you okay with
the patch as-is? (The asserts make it clear IMO.)

Thanks
Laszlo

> 
>>
>>
>>> ---
>>>  hw/acpi/vmgenid.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>> index c3ddcc8e7cb0..b5c0dfcf19e1 100644
>>> --- a/hw/acpi/vmgenid.c
>>> +++ b/hw/acpi/vmgenid.c
>>> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = {
>>>  static void vmgenid_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      VmGenIdState *vms = VMGENID(dev);
>>> +    Object *one_vmgenid;
>>> +    bool ambiguous;
>>>  
>>>      if (!vms->write_pointer_available) {
>>>          error_setg(errp, "%s requires DMA write support in fw_cfg, "
>>> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, Error 
>>> **errp)
>>>          return;
>>>      }
>>>  
>>> +    one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, &ambiguous);
>>> +    if (one_vmgenid == NULL) {
>>> +        assert(ambiguous);
>>> +        error_setg(errp, "at most one %s device is permitted", 
>>> VMGENID_DEVICE);
>>> +        return;
>>> +    }
>>> +    assert(one_vmgenid == OBJECT(vms));
>>> +
>>>      qemu_register_reset(vmgenid_handle_reset, vms);
>>>  }
>>>  
>>> -- 
>>> 2.9.3
>