[PULL,4/6] target-i386: Make "level" and "xlevel" properties static

Static properties require only 1 line of code, much simpler than the
existing code that requires writing new getters/setters.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 40 ++--------------------------------------
 1 file changed, 2 insertions(+), 38 deletions(-)

Am 27.04.2015 um 19:58 schrieb Eduardo Habkost:
> Static properties require only 1 line of code, much simpler than the
> existing code that requires writing new getters/setters.

This is missing the fact that there is a semantic difference between my
setters and those of your static properties:

> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 40 ++--------------------------------------
>  1 file changed, 2 insertions(+), 38 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 03b33cf..2bbf01d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1618,38 +1618,6 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
>      env->cpuid_version |= value & 0xf;
>  }
>  
> -static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque,
> -                                const char *name, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -
> -    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
> -}
> -
> -static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque,
> -                                const char *name, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -
> -    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
> -}

The setter of your static properties will prohibit setting the value
after the CPU has been realized.

If that is intended, you should at least mention it in the commit
message and not just argue with the amount of lines.

Regards,
Andreas

> -
> -static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque,
> -                                 const char *name, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -
> -    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
> -}
> -
> -static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque,
> -                                 const char *name, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -
> -    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
> -}
> -
>  static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> @@ -2900,12 +2868,6 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "stepping", "int",
>                          x86_cpuid_version_get_stepping,
>                          x86_cpuid_version_set_stepping, NULL, NULL, NULL);
> -    object_property_add(obj, "level", "int",
> -                        x86_cpuid_get_level,
> -                        x86_cpuid_set_level, NULL, NULL, NULL);
> -    object_property_add(obj, "xlevel", "int",
> -                        x86_cpuid_get_xlevel,
> -                        x86_cpuid_set_xlevel, NULL, NULL, NULL);
>      object_property_add_str(obj, "vendor",
>                              x86_cpuid_get_vendor,
>                              x86_cpuid_set_vendor, NULL);
> @@ -2998,6 +2960,8 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> +    DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> +    DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
>

On Mon, Apr 27, 2015 at 08:23:06PM +0200, Andreas Färber wrote:
> Am 27.04.2015 um 19:58 schrieb Eduardo Habkost:
> > Static properties require only 1 line of code, much simpler than the
> > existing code that requires writing new getters/setters.
> 
> This is missing the fact that there is a semantic difference between my
> setters and those of your static properties:
> 
> > 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 40 ++--------------------------------------
> >  1 file changed, 2 insertions(+), 38 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 03b33cf..2bbf01d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1618,38 +1618,6 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
> >      env->cpuid_version |= value & 0xf;
> >  }
> >  
> > -static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque,
> > -                                const char *name, Error **errp)
> > -{
> > -    X86CPU *cpu = X86_CPU(obj);
> > -
> > -    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
> > -}
> > -
> > -static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque,
> > -                                const char *name, Error **errp)
> > -{
> > -    X86CPU *cpu = X86_CPU(obj);
> > -
> > -    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
> > -}
> 
> The setter of your static properties will prohibit setting the value
> after the CPU has been realized.
> 
> If that is intended, you should at least mention it in the commit
> message and not just argue with the amount of lines.

That's a bug in all the existing setters in target-i386/cpu.c, and I
wasn't aware of the bug until a few days ago (long after this patch was
submitted). One of my work in progress branches includes a patch that
fixes all setters to check if the CPU is already realized.

I will update the commit message to mention the bug fix and re-send the
pull request.