Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init()

[Mark Cave-Ayland]

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.