On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote:
On Tue, 24 May 2016 09:34:05 -0300 Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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?
- } 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) {
-- Eduardo