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

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> 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.

Sure:

Signed-off-by: Eduardo Habkost <address@hidden>

> 
> > /* 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