Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Eduardo Habkost]

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 <address@hidden> 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).

-- 
Eduardo