qemu-devel
[Top][All Lists]
Advanced

[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: Fri, 7 Jul 2017 17:20:25 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

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.

>>  * from fw_cfg_common_realize() just call
>>
>>      // if fw_cfg_find() returns NULL it means that device isn't in QOM tree
>>      // which shouldn't ever happen, fw_cfg_find() will abort itself if
>>      // another instance of device present in QOM tree.
>>      assert(fw_cfg_find());
> 
> That would work, but I don't see why doing that if just returning
> NULL would: 1) make the code in fw_cfg_find() simpler and
> shorter; 2) make realize error handling friendlier (returning
> error instead of aborting).  We just need to document that
> explicitly in fw_cfg_find() (see find_vmgenid_dev() for an
> example).
> 
> If you still want to make realize abort instead of returning
> error, you don't even need assert(ambiguous) on fw_cfg_find().
> All you need is this on realize:
> 
>    assert(fw_cfg_find() == dev);

I'm definitely not a fan of the assert()...


ATB,

Mark.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]