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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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 <ehabkost@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-ayland@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.