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

["Andreas Färber" <afaerber@suse.de>]

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()
>  };
>  
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)