On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> 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.
Sure:
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
>
> > /* 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
> > >
> >
>
--
Eduardo
