On 07/07/17 16:07, Eduardo Habkost wrote: >> looks fine, >> >> so what I'd do is: >> * drop 4/6
Yes. > Agreed on this point. But: > >> * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == >> true During my latest tests I've found that everything works fine without the ambiguous argument. Do we still want to keep it? And I don't think error_abort() is the right thing to do here, I'd much rather return NULL and add a suitable comment. >> * from fw_cfg_common_realize() just call >> >> // if fw_cfg_find() returns NULL it means that device isn't in QOM tree >> // which shouldn't ever happen, fw_cfg_find() will abort itself if >> // another instance of device present in QOM tree. >> assert(fw_cfg_find()); > > That would work, but I don't see why doing that if just returning > NULL would: 1) make the code in fw_cfg_find() simpler and > shorter; 2) make realize error handling friendlier (returning > error instead of aborting). We just need to document that > explicitly in fw_cfg_find() (see find_vmgenid_dev() for an > example). > > If you still want to make realize abort instead of returning > error, you don't even need assert(ambiguous) on fw_cfg_find(). > All you need is this on realize: > > assert(fw_cfg_find() == dev); I'm definitely not a fan of the assert()... ATB, Mark.
