From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44554) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TlQWE-0005KO-6A for qemu-devel@nongnu.org; Wed, 19 Dec 2012 15:45:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TlQWD-0008BI-01 for qemu-devel@nongnu.org; Wed, 19 Dec 2012 15:45:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35926) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TlQWC-0008BA-Mk for qemu-devel@nongnu.org; Wed, 19 Dec 2012 15:45:32 -0500 Date: Wed, 19 Dec 2012 21:45:26 +0100 From: Igor Mammedov Message-ID: <20121219214526.631fdf2f@thinkpad.mammed.net> In-Reply-To: <20121219175840.GP5334@otherpad.lan.raisama.net> References: <1355760092-18755-1-git-send-email-imammedo@redhat.com> <1355760092-18755-16-git-send-email-imammedo@redhat.com> <20121219175840.GP5334@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 15/20] target-i386: set custom 'xlevel' without intermediate x86_def_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Don@CloudSwitch.com, qemu-devel@nongnu.org, afaerber@suse.de On Wed, 19 Dec 2012 15:58:40 -0200 Eduardo Habkost wrote: > On Mon, Dec 17, 2012 at 05:01:27PM +0100, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov > > --- > > target-i386/cpu.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 0cb0931..714ae79 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, > > if (numvalue < 0x80000000) { > > numvalue += 0x80000000; > > } > > - x86_cpu_def->xlevel = numvalue; > > + val = g_strdup_printf("%u", numvalue); > > Why not just eliminate the string->integer conversion entirely, and move > it inside x86_cpuid_set_xlevel()? Silent fixup of an incorrect input value doesn't look like a candidate for making into property setter, more correct way is bailing out. But for compatibility sake we can't just remove it without giving userbase a warning. Perhaps candidate for next release notes. And when it's removed, this branch could be collapsed in catch-all(feat=val) branch + adding input value check to xlevel property setter. > > > + qdict_put(*props, featurestr, qstring_from_str(val)); > > If you made cpu_x86_parse_featurestr() set the properties directly on > the object instead of creating an intermediate directionary, it could > simply call object_property_set_int() (if for some reason it's > impossible to eliminate the string->int conversion above). I've explained in answer to [10/20] why. Besides normalizing input first to uniform set and then applying it to object using only one approach whether it via cpu_x86_set_props() or global properties seems to me more straightforward and reduces chance of error. and might allow completely get rid of cpu_x86_parse_featurestr() call in cpu_init() call chain. cpu_x86_parse_featurestr() should/might be called only once to create a set of properties that are applied later to each created CPU. > > > + g_free(val); > > } else if (!strcmp(featurestr, "vendor")) { > > qdict_put(*props, featurestr, qstring_from_str(val)); > > x86_cpu_def->vendor_override = true; > > -- > > 1.7.1 > > > > > > -- > Eduardo -- Regards, Igor