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

[Eduardo Habkost]

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