On 01/12/17 14:10, Eduardo Habkost wrote:
> On Wed, Jan 11, 2017 at 06:34:55PM +0100, Laszlo Ersek wrote:
> [...]
>> static Property fw_cfg_io_properties[] = {
>> DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>> DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>> DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>> true),
>> + DEFINE_PROP_UINT16("file_slots", FWCfgIoState, parent_obj.file_slots,
>> + FW_CFG_FILE_SLOTS_MIN),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -1027,6 +1066,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error
>> **errp)
>
> I'm not sure what is more important here: following the QOM
> property name convention using "-" instead of "_", or being
> consistent with the other existing properties.
Right, when working with the compat settings, I saw both schemes. I
couldn't decide. I figured I'd stick with the underscore seen with
"dma_enabled".
>
> In either case, we could add a "x-" prefix to indicate it is not
> supposed to be configured directly by the user.
I thought "x-" meant "experimental"; i.e., the property could be changed
or could go away at any moment. That's a slightly different meaning than
"not meant for users".
BTW I think the same (== not meant for the user) applies to a number of
other properties (dma_enabled is one of them, but other devices have
this kind too); do we consistently call them x-*?
>
> [...]
>> @@ -1063,6 +1109,8 @@ static Property fw_cfg_mem_properties[] = {
>> DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>> DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>> true),
>> + DEFINE_PROP_UINT16("file_slots", FWCfgMemState, parent_obj.file_slots,
>> + FW_CFG_FILE_SLOTS_MIN),
>
> It looks like you can add the property to the TYPE_FW_CFG parent
> class instead of duplicating it on the subclasses. The existing
> "dma_enabled" property could be moved there as well.
I would prefer if someone could pick up that task, separately.
The idea crossed my mind when I was working on the common base class,
originally, wrt. dma_enabled. I vaguely remember that there was some
problem with moving up the property. Ultimately the reviewers didn't
expect me, or suggest, to move it up, hence the current status.
I think it's out of scope for this series. If you (or someone else) can
do it, I agree it would be an improvement, but I'd rather learn the
method from someone else's patch than experiment with it myself.
If there's a consensus on the x- prefix and/or the underscore/hyphen
question, I'm happy to send a v6 with those changes.
Thanks,
Laszlo
