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

[Eduardo Habkost <ehabkost@redhat.com>]

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.

-- 
Eduardo