Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 1 Jun 2016 15:46:20 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@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
> >   
>