On Wed, 1 Jun 2016 15:46:20 -0300 Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > --- > > target-i386/cpu.c | 32 +++++++++++++++++--------------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 238f69d..618aef9 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, > > char *features, > > X86CPU *cpu = X86_CPU(cs); > > char *featurestr; /* Single 'key=value" string being parsed */ > > uint32_t numvalue; > > + char num[32]; > > + const char *name; > > + char *err; > > Error *local_err = NULL; > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > while (featurestr) { > > - char *val; > > + char *val = NULL; > > if (featurestr[0] == '+') { > > add_flagname_to_bitmaps(featurestr + 1, plus_features, > > &local_err); > > } else if (featurestr[0] == '-') { > > @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, > > char *features, > > } else if ((val = strchr(featurestr, '='))) { > > *val = 0; val++; > > feat2prop(featurestr); > > + name = featurestr; > > if (!strcmp(featurestr, "xlevel")) { > > - char *err; > > - char num[32]; > > - > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, > > char *features, > > numvalue += 0x80000000; > > } > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > - object_property_parse(OBJECT(cpu), num, featurestr, > > &local_err); > > + val = num; > > } else if (!strcmp(featurestr, "tsc-freq")) { > > int64_t tsc_freq; > > - char *err; > > - char num[32]; > > > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > > QEMU_STRTOSZ_DEFSUFFIX_B, > > 1000); > > @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, > > char *features, > > return; > > } > > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > > - object_property_parse(OBJECT(cpu), num, "tsc-frequency", > > - &local_err); > > + val = num; > > + name = "tsc-frequency"; > > } else if (!strcmp(featurestr, "hv-spinlocks")) { > > - char *err; > > const int min = 0xFFF; > > - char num[32]; > > + > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, > > char *features, > > numvalue = min; > > } > > snprintf(num, sizeof(num), "%" PRId32, numvalue); > > - object_property_parse(OBJECT(cpu), num, featurestr, > > &local_err); > > - } else { > > - object_property_parse(OBJECT(cpu), val, featurestr, > > &local_err); > > + val = num; > > } > > } else { > > feat2prop(featurestr); > > - object_property_parse(OBJECT(cpu), "on", featurestr, > > &local_err); > > + name = featurestr; > > + val = (char *)"on"; > > You don't need this hack if you do something like this: > > > while (featurestr) { > - char *val = NULL; > + const char *val = NULL; > + char *eq = NULL; > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, plus_features, > &local_err); > } else if (featurestr[0] == '-') { > add_flagname_to_bitmaps(featurestr + 1, minus_features, > &local_err); > - } else if ((val = strchr(featurestr, '='))) { > - *val = 0; val++; > + } else if ((eq = strchr(featurestr, '='))) { > + *eq++ = 0; > + val = eq; > feat2prop(featurestr); > name = featurestr; > if (!strcmp(featurestr, "xlevel")) { > > > > } > > + > > + if (val) { > > + object_property_parse(OBJECT(cpu), val, name, &local_err); > > + } > > I would find it easier to read if this part was inside the same > block as the code that sets name/val. > > I would find it even better if object_property_parse() was in the > main loop body, and the +feature/-feature code placed inside an > if/continue branch (to make it clear that it is an exception, not > the rule). Like this: Thanks, I'll take your variant with your permission. > /* untested */ > static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > Error **errp) > { > X86CPU *cpu = X86_CPU(cs); > char *featurestr; /* Single 'key=value" string being parsed */ > char *err; > Error *local_err = NULL; > > if (!features) { > return; > } > > for (featurestr = strtok(features, ","); > featurestr; > featurestr = strtok(NULL, ",")) { > const char *name; > const char *val = NULL; > char *eq = NULL; > uint32_t numvalue; > char num[32]; > > /* Compatibility syntax: */ > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, plus_features, > &local_err); > continue; > } else if (featurestr[0] == '-') { > add_flagname_to_bitmaps(featurestr + 1, minus_features, > &local_err); > continue; > } > > eq = strchr(featurestr, '='); > if (eq) { > *eq++ = 0; > val = eq; > } else { > val = "on"; > } > > feat2prop(featurestr); > name = featurestr; > > /* Special cases: */ > if (!strcmp(name, "xlevel")) { > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > error_setg(errp, "bad numerical value %s", val); > return; > } > if (numvalue < 0x80000000) { > error_report("xlevel value shall always be >= 0x80000000" > ", fixup will be removed in future versions"); > numvalue += 0x80000000; > } > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > val = num; > } else if (!strcmp(name, "tsc-freq")) { > int64_t tsc_freq; > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > QEMU_STRTOSZ_DEFSUFFIX_B, 1000); > if (tsc_freq < 0 || *err) { > error_setg(errp, "bad numerical value %s", val); > return; > } > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > val = num; > name = "tsc-frequency"; > } else if (!strcmp(name, "hv-spinlocks")) { > const int min = 0xFFF; > > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > error_setg(errp, "bad numerical value %s", val); > return; > } > if (numvalue < min) { > error_report("hv-spinlocks value shall always be >= 0x%x" > ", fixup will be removed in future versions", > min); > numvalue = min; > } > snprintf(num, sizeof(num), "%" PRId32, numvalue); > val = num; > } > > object_property_parse(OBJECT(cpu), val, name, &local_err); > > if (local_err) { > error_propagate(errp, local_err); > return; > } > } > } > > > > + > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > -- > > 1.8.3.1 > > >