Patchwork [01/22] target-i386: return Error from cpu_x86_find_by_name()

login
register
mail settings
Submitter Igor Mammedov
Date Sept. 7, 2012, 8:54 p.m.
Message ID <1347051311-16122-2-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/182453/
State New
Headers show

Comments

Igor Mammedov - Sept. 7, 2012, 8:54 p.m.
it will allow to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
--
v2:
    style change, add braces (reqested by Blue Swirl)
---
 target-i386/cpu.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
Don Slutz - Sept. 11, 2012, 7:41 p.m.
On 09/07/12 16:54, Igor Mammedov wrote:
> it will allow to use property setters there later.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> --
> v2:
>      style change, add braces (reqested by Blue Swirl)
> ---
>   target-i386/cpu.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ac12139..a89bdc4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>       cpu->env.tsc_khz = value / 1000;
>   }
>   
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> +                                const char *cpu_model, Error **errp)
>   {
>       unsigned int i;
>       x86_def_t *def;
> @@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>               fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
>               goto error;
>           }
> +
> +        if (error_is_set(errp)) {
> +            goto error;
> +        }
> +
>           featurestr = strtok(NULL, ",");
>       }
>       x86_cpu_def->features |= plus_features;
> @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>   
>   error:
>       g_free(s);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +    }
>       return -1;
>   }
>   
> @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>   
>       memset(def, 0, sizeof(*def));
>   
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>       if (def->vendor1) {
>           env->cpuid_vendor1 = def->vendor1;
>           env->cpuid_vendor2 = def->vendor2;
> @@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>           env->cpuid_svm_features &= TCG_SVM_FEATURES;
>       }
>       object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +
> +out:
>       if (error_is_set(&error)) {
>           error_free(error);
>           return -1;
Reviewed-by: Don Slutz <Don@CloudSwitch.com>
Eduardo Habkost - Sept. 13, 2012, 2:38 p.m.
On Fri, Sep 07, 2012 at 10:54:50PM +0200, Igor Mammedov wrote:
> it will allow to use property setters there later.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> --
> v2:
>     style change, add braces (reqested by Blue Swirl)
> ---
>  target-i386/cpu.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ac12139..a89bdc4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> +                                const char *cpu_model, Error **errp)
>  {
>      unsigned int i;
>      x86_def_t *def;
> @@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
>              goto error;
>          }
> +
> +        if (error_is_set(errp)) {
> +            goto error;
> +        }
> +

Why add this check, if you never use errp on any code above that line
(and eventually all the code above gets removed by patch 19)?

But is harmless (as error is never set anyway), and this is code that
simply gets removed later in this series. So:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>          featurestr = strtok(NULL, ",");
>      }
>      x86_cpu_def->features |= plus_features;
> @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>  
>  error:
>      g_free(s);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +    }
>      return -1;
>  }
>  
> @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      memset(def, 0, sizeof(*def));
>  
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>      if (def->vendor1) {
>          env->cpuid_vendor1 = def->vendor1;
>          env->cpuid_vendor2 = def->vendor2;
> @@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          env->cpuid_svm_features &= TCG_SVM_FEATURES;
>      }
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +
> +out:
>      if (error_is_set(&error)) {
>          error_free(error);
>          return -1;
> -- 
> 1.7.11.4
> 
>
Igor Mammedov - Sept. 13, 2012, 3:49 p.m.
On Thu, 13 Sep 2012 11:38:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Sep 07, 2012 at 10:54:50PM +0200, Igor Mammedov wrote:
> > it will allow to use property setters there later.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > --
> > v2:
> >     style change, add braces (reqested by Blue Swirl)
> > ---
> >  target-i386/cpu.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ac12139..a89bdc4 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      cpu->env.tsc_khz = value / 1000;
> >  }
> >  
> > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> > +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> > +                                const char *cpu_model, Error **errp)
> >  {
> >      unsigned int i;
> >      x86_def_t *def;
> > @@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> >              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> >              goto error;
> >          }
> > +
> > +        if (error_is_set(errp)) {
> > +            goto error;
> > +        }
> > +
> 
> Why add this check, if you never use errp on any code above that line
> (and eventually all the code above gets removed by patch 19)?
It looks like remnants from initial version where each converted feature had
setter in this loop. 
I'll remove it for next respin.

> 
> But is harmless (as error is never set anyway), and this is code that
> simply gets removed later in this series. So:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> >          featurestr = strtok(NULL, ",");
> >      }
> >      x86_cpu_def->features |= plus_features;
> > @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> >  
> >  error:
> >      g_free(s);
> > +    if (!error_is_set(errp)) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> > +    }
> >      return -1;
> >  }
> >  
> > @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >  
> >      memset(def, 0, sizeof(*def));
> >  
> > -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> > -        return -1;
> > +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> > +        goto out;
> > +    }
> > +
> >      if (def->vendor1) {
> >          env->cpuid_vendor1 = def->vendor1;
> >          env->cpuid_vendor2 = def->vendor2;
> > @@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          env->cpuid_svm_features &= TCG_SVM_FEATURES;
> >      }
> >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> > +
> > +out:
> >      if (error_is_set(&error)) {
> >          error_free(error);
> >          return -1;
> > -- 
> > 1.7.11.4
> > 
> > 
> 
> -- 
> Eduardo

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ac12139..a89bdc4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,7 +1086,8 @@  static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1241,6 +1242,11 @@  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
+
+        if (error_is_set(errp)) {
+            goto error;
+        }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1264,6 +1270,9 @@  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1350,8 +1359,10 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1401,6 +1412,8 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;