[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_c
From: |
Mark Cave-Ayland |
Subject: |
Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers |
Date: |
Tue, 11 Jul 2017 19:05:16 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
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 <address@hidden> 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 <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).
>> 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.
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, (continued)
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Mark Cave-Ayland, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers,
Mark Cave-Ayland <=
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Laszlo Ersek, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Mark Cave-Ayland, 2017/07/07