From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7Imz-0005b3-49 for qemu-devel@nongnu.org; Mon, 30 May 2016 04:43:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b7Imu-0001fA-Ri for qemu-devel@nongnu.org; Mon, 30 May 2016 04:43:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42316) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7Imu-0001ey-JZ for qemu-devel@nongnu.org; Mon, 30 May 2016 04:43:04 -0400 Date: Mon, 30 May 2016 10:43:00 +0200 From: Igor Mammedov Message-ID: <20160530104300.78d2f596@nial.brq.redhat.com> In-Reply-To: <20160527203234.GL23701@thinpad.lan.raisama.net> References: <1462558292-2126-1-git-send-email-ehabkost@redhat.com> <1462558292-2126-9-git-send-email-ehabkost@redhat.com> <20160524141703.3379d7fe@nial.brq.redhat.com> <20160524123405.GD23701@thinpad.lan.raisama.net> <20160524152227.7f933c59@nial.brq.redhat.com> <20160527203234.GL23701@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Jiri Denemark , Andreas =?UTF-8?B?RsOkcmJlcg==?= , libvir-list@redhat.com On Fri, 27 May 2016 17:32:34 -0300 Eduardo Habkost wrote: > On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote: > > On Tue, 24 May 2016 09:34:05 -0300 > > Eduardo Habkost wrote: > > > > > On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote: > > > > On Fri, 6 May 2016 15:11:31 -0300 > > > > Eduardo Habkost wrote: > [...] > > > > > -/* Convert all '_' in a feature string option name to '-', to make feature > > > > > - * name conform to QOM property naming rule, which uses '-' instead of '_'. > > > > > +/* Convert all '_' in a feature string option name to '-', to keep compatibility > > > > > + * with old feature names that used "_" instead of "-". > > > > > */ > > > > > static inline void feat2prop(char *s) > > > > > { > > > > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > > > > while (featurestr) { > > > > > char *val; > > > > I'd place a single feat2prop() here > > > > and delete it from other call sites in this function. > > > > > > A previous version of this patch had it. But it would change the > > > property value too, not just the property name (breaking stuff > > > like "model-id=some_string"). > > > > > it's bug in feat2prop(), which probably should be fixed there, > > so it would do what comment above it says. Or as alternative: > > The comment above it doesn't say anything about stopping at a '=' > delimiter. I always expected it to just replace "_" with "-" in a > null-terminated string. > > (I am not completely against making it stop at '=', but I believe > your suggestion below sounds better). > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index ca2a893..e46e4c3 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features > > featurestr = features ? strtok(features, ",") : NULL; > > > > while (featurestr) { > > - char *val; > > + char *val = strchr(featurestr, '='); > > + if (val) { > > + *val = 0; val++; > > + } > > + feat2prop(featurestr); > > This would make "+feature=FOO" treated as a valid option, and it > isn't. It would keep the existing behavior if we did this: > > - if (featurestr[0] == '+') { > + if (featurestr[0] == '+' && !val) { > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > - } else if (featurestr[0] == '-') { > + if (featurestr[0] == '+' && !val) { > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > > In either case, I prefer to get this optimization reviewed as a > separate patch. Can you send it as a follow-up? sure > > > - } else if ((val = strchr(featurestr, '='))) { > > - *val = 0; val++; > > - feat2prop(featurestr); > > + } else if (val) { > > if (!strcmp(featurestr, "xlevel")) { > > char *err; > > char num[32]; > > @@ -2000,7 +2002,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > object_property_parse(OBJECT(cpu), val, featurestr, &local_err); > > } > > } else { > > - feat2prop(featurestr); > > object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); > > } > > if (local_err) { > > > > >