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

[Igor Mammedov]

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.