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

[Eduardo Habkost]

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:

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