On 21/06/17 12:36, Eduardo Habkost wrote: > On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote: >> On 06/21/17 08:58, Mark Cave-Ayland wrote: >>> On 19/06/17 23:43, Laszlo Ersek wrote: >>> >>>> It looks good to me, but please await Eduardo's reply as well. >>>> >>>> In particular, it should be confirmed that object_resolve_path_type() >>>> matches instances of *subclasses* as well (as I expect it would). Your >>>> test results confirm that; let's make sure it is intentional behavior. >>>> Eduardo (or someone else on the CC list), can you please comment on that? >>> >>> Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour? > > Sorry for taking so long to reply. Yes, it should be the correct > behavior. It's how it's documented: > > "This is similar to object_resolve_path. However, when looking > for a partial path only matches that implement the given type are > considered. This restricts the search and avoids spuriously > flagging matches as ambiguous." > > (Key part here is "implement the given type"). > > Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more > than one vmgenid device" looks good to me.
That is great news, thanks for confirming this. >>> >>> I now have a v7 patchset ready to go (currently hosted at >>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, >>> I've currently left off your Tested-by tag since I'm not sure it's still >>> valid for less-than-trivial changes - if you're happy for me to re-add >>> it before I send the v7 patchset to the list, please let me know. >> >> I intend to test v7 when you post it. > > I still see the instance_init assert() in that branch (commit > 17d75643f880). Is that correct? Yes that was the intention. In 17d75643f880 both the assert() and object_property_add_child() are moved into the instance_init() function, and then in the follow-up commit eddedb5 the assert() is removed from instance_init() and the object_resolve_path_type() check added into fw_cfg_init1() as part of its conversion into the fw_cfg_common_realize() function. One last question which might avoid a future v8 revision: does the error handling in eddedb5 for the fw_cfg_*_realize() functions look correct? The existing check for fw_cfg_file_slots_allocate() uses a local_err Error variable, whereas I've seen a mixture of callers using both this approach and using the errp Error variable directly. Is there any reason to prefer one over the other? Currently I'm also using local_err in order to keep the fw_cfg_*_realize() functions consistent. ATB, Mark.
