Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 8 Apr 2015 12:01:26 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 08, 2015 at 04:09:32PM +0200, Igor Mammedov wrote:
> > On Wed, 08 Apr 2015 14:24:35 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On 08/04/2015 14:20, Eduardo Habkost wrote:
> > > >> > it would be better to create generic bit property and replace above code with it
> > > >> > something similar to object_property_add_uint32_ptr()
> > > > object_property_add_*_ptr() adds read-only properties, and I didn't want
> > > > to make object_property_add_bit_ptr() inconsistent with the other
> > > > functions. But maybe it is better to have an inconsistent but reusable
> > > > API than making the new code non-reusable by keeping it inside
> > > > target-i386/cpu.c. I will give it a try.
> > > 
> > > add_*_ptr() is read-only because read-only properties do not require
> > > validations (at least that's the common case).  So I think the
> > > inconsistent API is worse than a local one.
> > I've had on my todo list adding add_*_ptr_RW() variants
> > because in common case there isn't any need in validation and
> > with current API we are forced to duplicate custom setter/getter
> > every time for trivial type.
> > For example properties like "level" or "xlevel".
> > 
> > The same goes for generic bitmap setter (i.e.) without validation,
> > which is what we need in this case. We [could] do validation at realize
> > time when it's possible to analyze combination of feature flags.
> 
> Even if no property value validation is done at the setter, we still
> want to prevent the property from being changed after realize (which is
> something I forgot to do in this patch, and we forgot to do on most
> X86CPU setters).
Yep, the same goes for every generic setters in current API
since realize is device specific thingy that's why it's supported by
static properties only now.
We should have something like Stefan's object_property_add_link(...,*check,...)
but for generic properties.