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

[Mark Cave-Ayland]

On 07/07/17 14:13, Eduardo Habkost wrote:

>>> This should be the same behaviour as before, i.e. a NULL means that the
>>> fw_cfg device couldn't be found. It seems intuitive to me from the name
>>> of the function exactly what it does, so I don't find the lack of
>>> comment too confusing. Does anyone else have any thoughts here?
>> intuitive meaning from the function name would be return NULL if nothing 
>> were found,
>> however it's not the case here.
>>
>> taking in account that fwcfg in not user creatable device how about:
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 316fca9..8f6eef8 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1014,7 +1014,10 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr 
>> data_addr)
>>  
>>  FWCfgState *fw_cfg_find(void)
>>  {
>> -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>> +    bool ambig = false;
>> +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, 
>> &ambig));
>> +    assert(!ambig);
>> +    return f;
>>  }
>>
>> or if we must to print user friendly error and fail realize gracefully
>> (not sure why) just add errp argument to function so it could report
>> error back to caller, then places that do not care much about graceful
>> exit would use error_abort as argument and realize would use
>> its local_error/errp argument.
> 
> I don't disagree with adding the assert(), but it looks like
> making fw_cfg_find() return NULL if there are multiple devices
> can be useful for realize.
> 
> In this case, it looks like Mark is relying on that in
> fw_cfg_common_realize(): if multiple devices are created,
> fw_cfg_find() will return NULL, and realize will fail.  This
> sounds like a more graceful way to handle multiple-device
> creation than crashing on fw_cfg_find().  This is the solution
> used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> 
> Either way, we have to choose: either we make fw_cfg_find() crash
> when multiple devices exist (in this case, the fw_cfg_find() call
> on realize will be useless), or we make it return NULL and
> document it very clearly.

My personal preference too would be to keep the existing behaviour i.e.
NULL indicates failure and add a comment explaining how this also
matches the behaviour of object_resolve_path_type(). Then it is up the
caller to decide how to handle the severity as they wish.

>>>>> +        error_setg(errp, "at most one %s device is permitted", 
>>>>> TYPE_FW_CFG);  
>>>> s/TYPE_FW_CFG/object_get_typename()/
>>>> so it would print leaf type name   
> 
> I disagree.  We allow at most one fw_cfg device (it doesn't
> matter which type), not at most one device of each leaf type.
> Saying "at most one fw_cfg_mem device is permitted" if 1
> fw_cfg_io and 1 fw_cfg_mem device is created would be misleading.

Yes, I agree with you here. FWIW I've just done some more testing with
patch 4 dropped and it seems to be working well, i.e. I can't manage to
reproduce the failure case I was seeing before. Slightly annoying, but
also promising.


ATB,

Mark.