On Wed, Jun 21, 2017 at 01:17:06PM +0100, Mark Cave-Ayland wrote: > 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.
We can't move assert() + linking to instance_init even if it's just temporary, as it makes device-list-properties crash. Just linking in instance_init is also a problem, because instance_init should never affect global QEMU state. You could move linking to realize as well, but: just like you already moved sysbus_add_io() calls outside realize, I believe linking belongs outside realize too. I need to re-read the discussion threads to understand the motivation behind that. > > 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 error handling looks correct to me. > > 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. You need a local_err variable if you need to handle/check for errors before propagating them. Quoting qapi/error.h: Create a new error and pass it to the caller: error_setg(errp, "situation normal, all fouled up"); Call a function and receive an error from it: Error *err = NULL; foo(arg, &err); if (err) { handle the error... } [...] Receive an error and pass it on to the caller: Error *err = NULL; foo(arg, &err); if (err) { handle the error... error_propagate(errp, err); } where Error **errp is a parameter, by convention the last one. Do *not* "optimize" this to foo(arg, errp); if (*errp) { // WRONG! handle the error... } because errp may be NULL! But when all you do with the error is pass it on, please use foo(arg, errp); for readability. In fw_cfg_*_realize() you can call fw_cfg_common_realize(dev, errp) directly, but it won't be a problem if you use local_err just to keep it consistent with the other error checks in the function. -- Eduardo