On Mon, Jun 06, 2016 at 05:16:47PM +0200, Igor Mammedov wrote: > From: Eduardo Habkost <ehabk...@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > Reviewed-by: Igor Mammedov <imamm...@redhat.com> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> A small suggestion in case we are going to need a new version of this series, below: > --- > v1: > - fix error handling in of +-feat, Igor Mammedov <imamm...@redhat.com> > - rebase on top of > "target-i386: Remove xlevel & hv-spinlocks option fixups" > --- > target-i386/cpu.c | 74 > ++++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 25 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 349b971..f791a06 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1957,43 +1957,67 @@ static void x86_cpu_parse_featurestr(CPUState *cs, > char *features, > char *featurestr; /* Single 'key=value" string being parsed */ > Error *local_err = NULL; > > - featurestr = features ? strtok(features, ",") : NULL; > + if (!features) { > + return; > + } > > - while (featurestr) { > - char *val; > + for (featurestr = strtok(features, ","); > + featurestr; > + featurestr = strtok(NULL, ",")) { > + const char *name; > + const char *val = NULL; > + char *eq = NULL; > + > + /* Compatibility syntax: */ > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, plus_features, > &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + continue; If you add an error_propagate() call to the end of the function, this can be shortened to: if (local_err) { break; } Or maybe the loop could be simply written as: for (featurestr = strtok(features, ","); featurestr && !local_err; featurestr = strtok(NULL, ",")) and we could avoid all the if (local_err) checks inside the loop body. > } else if (featurestr[0] == '-') { > add_flagname_to_bitmaps(featurestr + 1, minus_features, > &local_err); > - } else if ((val = strchr(featurestr, '='))) { > - *val = 0; val++; > - feat2prop(featurestr); > - 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); > - if (tsc_freq < 0 || *err) { > - error_setg(errp, "bad numerical value %s", val); > - return; > - } > - snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > - object_property_parse(OBJECT(cpu), num, "tsc-frequency", > - &local_err); > - } else { > - object_property_parse(OBJECT(cpu), val, featurestr, > &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > } > + continue; > + } > + > + eq = strchr(featurestr, '='); > + if (eq) { > + *eq++ = 0; > + val = eq; > } else { > - feat2prop(featurestr); > - object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); > + val = "on"; > + } > + > + feat2prop(featurestr); > + name = featurestr; > + > + /* Special case: */ > + if (!strcmp(name, "tsc-freq")) { > + int64_t tsc_freq; > + char *err; > + char num[32]; > + > + 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"; > } > + > + object_property_parse(OBJECT(cpu), val, name, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } > - featurestr = strtok(NULL, ","); > } > } > > -- > 1.8.3.1 > -- Eduardo