[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 4/6] target-i386: Make "level" and "xlevel" prope
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PULL 4/6] target-i386: Make "level" and "xlevel" properties static |
Date: |
Mon, 27 Apr 2015 15:27:46 -0300 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > 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
- [Qemu-devel] [PULL 0/6] X86 patches, Eduardo Habkost, 2015/04/27
- [Qemu-devel] [PULL 1/6] MAINTAINERS: Add myself to X86, Eduardo Habkost, 2015/04/27
- [Qemu-devel] [PULL 2/6] MAINTAINERS: Change status of X86 to Maintained, Eduardo Habkost, 2015/04/27
- [Qemu-devel] [PULL 3/6] qemu-config: Accept empty option values, Eduardo Habkost, 2015/04/27
- [Qemu-devel] [PULL 5/6] target-i386: X86CPU::xlevel2 QOM property, Eduardo Habkost, 2015/04/27
- [Qemu-devel] [PULL 4/6] target-i386: Make "level" and "xlevel" properties static, Eduardo Habkost, 2015/04/27
- [Qemu-devel] [PULL 6/6] target-i386: Remove AMD feature flag aliases from CPU model table, Eduardo Habkost, 2015/04/27
- Re: [Qemu-devel] [PULL 0/6] X86 patches, Andreas Färber, 2015/04/27