qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH 05/10] target-i386: cpu: consolidate calls of obje


From: Eduardo Habkost
Subject: Re: [Qemu-arm] [PATCH 05/10] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
Date: Tue, 7 Jun 2016 17:37:48 -0300
User-agent: Mutt/1.6.1 (2016-04-27)

On Mon, Jun 06, 2016 at 05:16:47PM +0200, Igor Mammedov wrote:
> From: Eduardo Habkost <address@hidden>
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> Reviewed-by: Igor Mammedov <address@hidden>
> Signed-off-by: Igor Mammedov <address@hidden>

Reviewed-by: Eduardo Habkost <address@hidden>

A small suggestion in case we are going to need a new version of
this series, below:

> ---
> v1:
>  - fix error handling in of +-feat, Igor Mammedov <address@hidden>
>  - rebase on top of
>     "target-i386: Remove xlevel & hv-spinlocks option fixups"
> ---
>  target-i386/cpu.c | 74 
> ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 349b971..f791a06 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1957,43 +1957,67 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> char *features,
>      char *featurestr; /* Single 'key=value" string being parsed */
>      Error *local_err = NULL;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> +    if (!features) {
> +        return;
> +    }
>  
> -    while (featurestr) {
> -        char *val;
> +    for (featurestr = strtok(features, ",");
> +         featurestr;
> +         featurestr = strtok(NULL, ",")) {
> +        const char *name;
> +        const char *val = NULL;
> +        char *eq = NULL;
> +
> +        /* Compatibility syntax: */
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, 
> &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +            continue;

If you add an error_propagate() call to the end of the function,
this can be shortened to:

    if (local_err) {
        break;
    }

Or maybe the loop could be simply written as:

    for (featurestr = strtok(features, ",");
         featurestr && !local_err;
         featurestr = strtok(NULL, ","))

and we could avoid all the if (local_err) checks inside the loop
body.


>          } else if (featurestr[0] == '-') {
>              add_flagname_to_bitmaps(featurestr + 1, minus_features, 
> &local_err);
> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> -            feat2prop(featurestr);
> -            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);
> -                if (tsc_freq < 0 || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> -                    return;
> -                }
> -                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> -                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> -                                      &local_err);
> -            } else {
> -                object_property_parse(OBJECT(cpu), val, featurestr, 
> &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
>              }
> +            continue;
> +        }
> +
> +        eq = strchr(featurestr, '=');
> +        if (eq) {
> +            *eq++ = 0;
> +            val = eq;
>          } else {
> -            feat2prop(featurestr);
> -            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
> +            val = "on";
> +        }
> +
> +        feat2prop(featurestr);
> +        name = featurestr;
> +
> +        /* Special case: */
> +        if (!strcmp(name, "tsc-freq")) {
> +            int64_t tsc_freq;
> +            char *err;
> +            char num[32];
> +
> +            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";
>          }
> +
> +        object_property_parse(OBJECT(cpu), val, name, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
>          }
> -        featurestr = strtok(NULL, ",");
>      }
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]