On 10/07/17 18:38, Eduardo Habkost wrote: > 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.
I agree that it's also not necessary. The aim of the patch is to get the fw_cfg device to the point where it can be instantiated directly via qdev - and while the patch does use fw_cfg_find() to help with that, fw_cfg_find() has other callers too and I feel that it goes beyond the scope of the patch to start changing those semantics too. As freeze is coming up next week, I will post a v8 shortly. ATB, Mark.