Re: [Qemu-devel] [PATCH 4/5] target-i386: set custom features/properties without intermediate x86_def_t

[Igor Mammedov]

On Thu, 17 Jan 2013 15:44:45 -0200
Eduardo Habkost <address@hidden> wrote:

> On Thu, Jan 17, 2013 at 04:16:33PM +0100, Igor Mammedov wrote:
> > Move custom features parsing after built-in cpu_model defaults are set
> > and set custom features directly on CPU instance. That allows to make
> > clear distinction between built-in cpu model defaults that eventually
> > should go into clas_init() and extra property setting which is done
> > after defaults are set on CPU instance.
> > 
> > Impl. details:
> >  - features that are already properties, are converted to normalized
> >    (name, values) list. And after featurestr has been parsed,
> >    properties from the list are applied directly to CPU instance.
> >    * For now it provides uniform handling of properties with single
> >      object_property_parse() property setter.
> >    * And after current features/properties are converted into static
> >      properties, it will take a trivial patch to switch to global
> > properties. Which will allow to:
> >      * get CPU instance initialized with all parameters passed on -cpu ...
> >        cmd. line from object_new() call.
> >      * call cpu_model/featurestr parsing only once before CPUs are created
> >      * open a road for removing CPUxxxState.cpu_model_str field, when
> > other CPUs are similarly converted to subclasses and static properties.
> >  - re-factor error handling, to use Error instead of fprintf()s, since
> >    it is anyway passed in for property setter.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  target-i386/cpu.c |  144
> > ++++++++++++++++++++++++++++------------------------- 1 files changed, 77
> > insertions(+), 67 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 5cd7917..50e10b1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *name) return 0;
> >  }
> >  
> > +typedef struct NameValuePair {
> > +    char *name;
> > +    char *value;
> > +    QTAILQ_ENTRY(NameValuePair) next;
> > +} NameValuePair;
> > +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList;
> > +
> > +static void x86_cpu_add_nv_pair(NVList *list, const char *name,
> > +                                const char *value) {
> > +    NameValuePair *p = g_malloc0(sizeof(*p));
> > +    p->name = g_strdup(name);
> > +    p->value = g_strdup(value);
> > +    QTAILQ_INSERT_TAIL(list, p, next);
> > +}
> 
> I am not sure I like this extra complexity. We don't need it if we
> simply set/register the properties directly.
> 
> I have a different proposal:
> 
> 1) By now, use:
>       object_property_parse(OBJECT(cpu), P, V, errp)
>    in the places you are using:
>       x86_cpu_add_nv_pair(&props, P, V)
>    below.
> 
> 2) The day we move to global properties, we just need to mechanically
>    replace the occurrences of:
>      object_property_parse(OBJECT(cpu), P, V, errp)
>    with:
>      qdev_prop_register_global("X86CPU", P, V)
> 
> What do you think?
Agreed, changes, when switching to global properties, will be mechanical
and not in many places, so it makes sense to drop list approach.
I'll resubmit series.

> 
> 
> > +        object_property_parse(OBJECT(cpu), p->value, p->name, errp);
> > +
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
[...]