On 07/07/17 14:13, Eduardo Habkost wrote: >>> This should be the same behaviour as before, i.e. a NULL means that the >>> fw_cfg device couldn't be found. It seems intuitive to me from the name >>> of the function exactly what it does, so I don't find the lack of >>> comment too confusing. Does anyone else have any thoughts here? >> intuitive meaning from the function name would be return NULL if nothing >> were found, >> however it's not the case here. >> >> 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. > > I don't disagree with adding the assert(), but it looks like > making fw_cfg_find() return NULL if there are multiple devices > can be useful for realize. > > In this case, it looks like Mark is relying on that in > fw_cfg_common_realize(): if multiple devices are created, > fw_cfg_find() will return NULL, and realize will fail. This > sounds like a more graceful way to handle multiple-device > creation than crashing on fw_cfg_find(). This is the solution > used by find_vmgenid_dev()/vmgenid_realize(), BTW. > > Either way, we have to choose: either we make fw_cfg_find() crash > when multiple devices exist (in this case, the fw_cfg_find() call > on realize will be useless), or we make it return NULL and > document it very clearly.
My personal preference too would be to keep the existing behaviour i.e. NULL indicates failure and add a comment explaining how this also matches the behaviour of object_resolve_path_type(). Then it is up the caller to decide how to handle the severity as they wish. >>>>> + error_setg(errp, "at most one %s device is permitted", >>>>> TYPE_FW_CFG); >>>> s/TYPE_FW_CFG/object_get_typename()/ >>>> so it would print leaf type name > > I disagree. We allow at most one fw_cfg device (it doesn't > matter which type), not at most one device of each leaf type. > Saying "at most one fw_cfg_mem device is permitted" if 1 > fw_cfg_io and 1 fw_cfg_mem device is created would be misleading. Yes, I agree with you here. FWIW I've just done some more testing with patch 4 dropped and it seems to be working well, i.e. I can't manage to reproduce the failure case I was seeing before. Slightly annoying, but also promising. ATB, Mark.