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 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.