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

[Michael S. Tsirkin]

On Mon, Mar 20, 2017 at 05:22:16PM +0100, Laszlo Ersek wrote:
> 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.

I don't see this assumption anywhere in spec. What do you have in mind?

> 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

I prefer calling find_vmgenid_dev, and adding a comment
near find_vmgenid_dev.

> > 
> >>
> >>
> >>> ---
> >>>  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
> >