On Mon, Jul 10, 2017 at 05:23:36PM +0200, Igor Mammedov wrote: > On Mon, 10 Jul 2017 11:53:31 -0300 > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > On Mon, Jul 10, 2017 at 10:01:47AM +0200, Igor Mammedov wrote: > > > On Fri, 7 Jul 2017 17:20:25 +0100 > > > Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> wrote: > > > > > > > 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. > > > I'd still use ambiguous argument and since you prefer not to assert > > > I'd add errp argument to fw_cfg_find() and handle error at callsites. > > > > > > Just returning NULL isn't sufficient if you need to distinguish > > > 'not found' vs 'duplicate' usecases, additionally 'not found' > > > in most cases isn't even error but 'duplicate' definitely is. > > > > > > Aborting on diplicate in fw_cfg_find() is fine and would > > > help to avoid touching current callers if you wish to limit > > > patches scope, but you can go with proper error propagating > > > route if you wish. > > > > Just making realize refuse to create two devices sounds much > > simpler to me. No need to make fw_cfg_find() more complex (if we > > add errp argument to it) or less useful (if we add > > assert(!ambiguous) to it). > the problem here was a error message to print if fw_cfg_find() > returns NULL for missing or duplicate, if we need to print > precise error we would need proper error handling.
I don't see where we would need a precise error message, except for realizefn (where the only case fw_cfg_find() would return NULL is for duplicate devices). > > Considering to fw_cfg is builtin device I'd prefer just > assert in fw_cfg_find() on duplicate (all the callers consider it as error) > and let developer to deal with assert if it is triggered. Except that it would make it more difficult for realizefn to return a proper error message. Anyway, I am not completely against adding assert(!ambiguous) to fw_cfg_find() if Mark wants to follow your advice. I just think it's not necessary. I will only continue discussing this if I see issues in the next version of the series. -- Eduardo