From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42216) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ymnl3-0007Xe-4A for qemu-devel@nongnu.org; Mon, 27 Apr 2015 14:27:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ymnkz-0004fd-51 for qemu-devel@nongnu.org; Mon, 27 Apr 2015 14:27:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39841) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ymnky-0004fR-WA for qemu-devel@nongnu.org; Mon, 27 Apr 2015 14:27:49 -0400 Date: Mon, 27 Apr 2015 15:27:46 -0300 From: Eduardo Habkost Message-ID: <20150427182746.GC17796@thinpad.lan.raisama.net> References: <1430157509-9486-1-git-send-email-ehabkost@redhat.com> <1430157509-9486-5-git-send-email-ehabkost@redhat.com> <553E7E8A.1070104@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <553E7E8A.1070104@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 4/6] target-i386: Make "level" and "xlevel" properties static List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Peter Maydell , Richard Henderson , qemu-devel@nongnu.org, Paolo Bonzini On Mon, Apr 27, 2015 at 08:23:06PM +0200, Andreas F=E4rber 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. >=20 > This is missing the fact that there is a semantic difference between my > setters and those of your static properties: >=20 > >=20 > > Reviewed-by: Igor Mammedov > > Signed-off-by: Eduardo Habkost > > --- > > target-i386/cpu.c | 40 ++-------------------------------------- > > 1 file changed, 2 insertions(+), 38 deletions(-) > >=20 > > 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(Obj= ect *obj, Visitor *v, > > env->cpuid_version |=3D value & 0xf; > > } > > =20 > > -static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaqu= e, > > - const char *name, Error **errp) > > -{ > > - X86CPU *cpu =3D X86_CPU(obj); > > - > > - visit_type_uint32(v, &cpu->env.cpuid_level, name, errp); > > -} > > - > > -static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaqu= e, > > - const char *name, Error **errp) > > -{ > > - X86CPU *cpu =3D X86_CPU(obj); > > - > > - visit_type_uint32(v, &cpu->env.cpuid_level, name, errp); > > -} >=20 > The setter of your static properties will prohibit setting the value > after the CPU has been realized. >=20 > 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. --=20 Eduardo