Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Eduardo Habkost]

On Mon, Jul 10, 2017 at 09:49:38AM +0200, Igor Mammedov wrote:
> On Fri, 7 Jul 2017 12:07:07 -0300
> "Eduardo Habkost" <address@hidden> wrote:
> 
> > On Fri, Jul 07, 2017 at 04:44:53PM +0200, Igor Mammedov wrote:
> > [...]
> > > > > taking in account that fwcfg in not user creatable device how about:
> > > > > 
> > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > > > > index 316fca9..8f6eef8 100644
> > > > > --- a/hw/nvram/fw_cfg.c
> > > > > +++ b/hw/nvram/fw_cfg.c
> > > > > @@ -1014,7 +1014,10 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, 
> > > > > hwaddr data_addr)
> > > > >  
> > > > >  FWCfgState *fw_cfg_find(void)
> > > > >  {
> > > > > -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> > > > > +    bool ambig = false;
> > > > > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, 
> > > > > &ambig));
> > > > > +    assert(!ambig);
> > > > > +    return f;
> > > > >  }
> > > > > 
> > > > > or if we must to print user friendly error and fail realize gracefully
> > > > > (not sure why) just add errp argument to function so it could report
> > > > > error back to caller, then places that do not care much about graceful
> > > > > exit would use error_abort as argument and realize would use
> > > > > its local_error/errp argument.    
> > > > 
> > > > My understanding from the thread was that we should only use assert()s
> > > > where there is no other choice so that any failures can be handled in a
> > > > more friendly manner.  
> > > the rule I use to decide assert vs nice error handling:
> > > 1: try to avoid crash on hotplug path
> > > 2: if error could be induced by end user on startup, try to print nice 
> > > error
> > >    before dying
> > > 3: when error should not happen just assert or use error_abort
> > >    which would print nice error message before dying.  
> > 
> > I would add this to the list:
> > 
> > If returning error instead of aborting is easy, return an error
> > and let the caller decide if it should use &error_abort or not.
> agreed, there aren't that many callers of fw_cfg_find()
> so I'd just add error argument to it and handle error at call sites.

I was talking about realizefn, which already has a errp
parameter.  IMO a errp parameter to fw_cfg_find() would be
overkill.

(And an assert(!ambiguous) would make fw_cfg_find() less useful
because then it won't be usable by realizefn).

-- 
Eduardo