Re: [Qemu-devel] [PATCH v2 05/45] qdev: remove PropertyInfo.qtype field

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 1 June 2017 at 12:19, Markus Armbruster <address@hidden> wrote:
>> Lovely cleanup.
>>
>> The interesting part is the move of the bits controlling use of ->defval
>> from Property member .qtype (set only by DEFINE_PROP_foo() macros) to
>> its PropertyInfo method .info->set_default_value().  No functional
>> change if (1) we preserve existing use of ->defval and (2) we don't add
>> new uses of ->defval, or at least not uses that matter.
>>
>> Direction (1) is obvious for DEFINE_PROP_BIT(), DEFINE_PROP_BIT64(),
>> DEFINE_PROP_BOOL().  They set .info to &qdev_prop_bit, &qdev_prop_bit64,
>> &qdev_prop_bool, respectively.  These all get .set_default_value =
>> set_default_value_bool, which preserves the effect of .qtype =
>> QTYPE_BOOL.  Good.
>>
>> DEFINE_PROP_DEFAULT() sets .info to point to its argument _prop.  The
>> following arguments occur:
>>
>> * In include/hw/qdev-properties.h: qdev_prop_uint8, qdev_prop_uint16,
>>   qdev_prop_uint32, qdev_prop_int32, qdev_prop_uint64, qdev_prop_size,
>>   qdev_prop_pci_devfn, qdev_prop_on_off_auto, qdev_prop_losttickpolicy,
>>   qdev_prop_blockdev_on_error, qdev_prop_bios_chs_trans,
>>   qdev_prop_blocksize
>>
>> * In hw/block/fdc.c: qdev_prop_fdc_drive_type
>>
>> * In hw/net/e1000e.c: local copies of qdev_prop_uint8, qdev_prop_uint16.
>>
>> To preserve the effect of .qtype = QTYPE_QINT, these PropertyInfo need
>> .set_default_value = set_default_value_int or .set_default_value =
>> set_default_value_enum, depending on .enum_table.  They get it.  Good.
>>
>> DEFINE_PROP_ARRAY() sets .info to &qdev_prop_arraylen.  Needs and gets
>> .set_default_value = set_default_value_int.  Good.
>>
>> DEFINE_PROP_ARRAY() also takes a PropertyInfo argument, but it's not
>> relevant here, as the .qtype you remove doesn't apply to it.  I'm
>> mentioning this, because it confused me briefly.
>>
>> Direction (2) isn't as visible in the patch.  For each PropertyInfo you
>> change, we need to find and check stores into .info not wrapped in the
>> DEFINE_PROP_foo() macros you change.
>>
>> I can't find any.  Good.
>
> I think this is going to break a patch I was halfway through
> writing :-(
>
> What I want is "DEFINE_PROP_UINT32, but don't set the default value"
> (because the default value in this case (a) isn't constant and
> (b) is already in the struct field that the property modifies).
> My plan for doing this was:
>  static Property arm_cpu_pmsav7_dregion_property =
>     DEFINE_PROP("pmsav7-dregion", ARMCPU, pmsav7_dregion,
>                 qdev_prop_uint32, uint32_t);
>
> This won't work any more if qdev_property_add_static
> assumes that "qdev_prop_uint32" implies "always set the default value".

After this patch, qdev_property_add_static() delegates assigning a
default value to prop->info->set_default_value(obj, prop).

qdev_prop_uint32.set_default_value() assigns prop->defval.

> So how should I obtain those semantics with this cleanup in place ?

Two ways come to mind:

* Define a PropertyInfo like qdev_prop_uint32 with a null
  set_default_value(), and use that.

* Add a flag to Property that makes qdev_property_add_static() skip
  prop->info->set_default_value(), set it for your property.

  Actually, I'd probably do it the other way: call ->set_default_value()
  only when the flag is set.  No need to check it's non-null then.
  Setting the flag when it's null is a programming error.

Could one of these two work for you?