[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr |
Date: |
Thu, 2 Jun 2016 14:22:22 +0200 |
On Wed, 1 Jun 2016 15:46:20 -0300
Eduardo Habkost <address@hidden> wrote:
> On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > 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
> >
>
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, (continued)
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Eduardo Habkost, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Igor Mammedov, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Eduardo Habkost, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Igor Mammedov, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Eduardo Habkost, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Igor Mammedov, 2016/06/03
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Eduardo Habkost, 2016/06/03
- Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time, Igor Mammedov, 2016/06/04
[Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Igor Mammedov, 2016/06/01
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Eduardo Habkost, 2016/06/01
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr,
Igor Mammedov <=
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Eduardo Habkost, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Eduardo Habkost, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Peter Krempa, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Igor Mammedov, 2016/06/02
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Peter Krempa, 2016/06/03
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Igor Mammedov, 2016/06/03
- Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Eduardo Habkost, 2016/06/03
Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr, Igor Mammedov, 2016/06/02
[Qemu-devel] [PATCH] target-i386: Remove xlevel & hv-spinlocks option fixups, Eduardo Habkost, 2016/06/02
Re: [Qemu-devel] [libvirt] [PATCH] target-i386: Remove xlevel & hv-spinlocks option fixups, Eric Blake, 2016/06/02