qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device
Date: Mon, 20 Mar 2017 16:13:42 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

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.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]