On Fri, 7 Jul 2017 12:07:07 -0300
"Eduardo Habkost" <ehabk...@redhat.com> 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.
