Patchwork [2/2] target-i386: make cpu_x86_create() get Error argument

login
register
mail settings
Submitter Eduardo Habkost
Date Dec. 12, 2012, 6:16 p.m.
Message ID <1355336183-18858-3-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/205597/
State New
Headers show

Comments

Eduardo Habkost - Dec. 12, 2012, 6:16 p.m.
Instead of forcing the caller to guess what went wrong while creating
the CPU object, return error information in a Error argument.

Also, as cpu_x86_create() won't print error messages itself anymore,
change cpu_x86_init() to print any error returned by cpu_x86_create()
or cpu_x86_realize().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c    |  8 ++++++--
 target-i386/cpu.h    |  2 +-
 target-i386/helper.c | 21 ++++++++++++++-------
 3 files changed, 21 insertions(+), 10 deletions(-)
Igor Mammedov - Dec. 14, 2012, 10:18 a.m.
On Wed, 12 Dec 2012 16:16:23 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of forcing the caller to guess what went wrong while creating
> the CPU object, return error information in a Error argument.
> 
> Also, as cpu_x86_create() won't print error messages itself anymore,
> change cpu_x86_init() to print any error returned by cpu_x86_create()
> or cpu_x86_realize().
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c    |  8 ++++++--
>  target-i386/cpu.h    |  2 +-
>  target-i386/helper.c | 21 ++++++++++++++-------
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b242bf1..fba872d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1542,7 +1542,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  /* Create and initialize a X86CPU object, based on the full CPU model string
>   * (that may include "+feature,-feature,feature=xxx" feature strings)
>   */
> -X86CPU *cpu_x86_create(const char *cpu_model)
> +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>  {
>      X86CPU *cpu;
>      CPUX86State *env;
> @@ -1559,12 +1559,14 @@ X86CPU *cpu_x86_create(const char *cpu_model)
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> +        error_setg(errp, "invalid CPU model string: %s", cpu_model);
>          goto error;
>      }
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
>      if (cpu_x86_find_by_name(def, name) < 0) {
> +        error_setg(errp, "CPU model not found: %s", name);
>          goto error;
>      }
>  
> @@ -1575,13 +1577,15 @@ X86CPU *cpu_x86_create(const char *cpu_model)
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> +        error_setg(errp, "Error parsing feature string: %s",
> +                   features ? features : "(none)");
It could be simplified, it shouldn't get here if features == NULL

>          goto error;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
>  
>      if (error) {
> -        fprintf(stderr, "%s\n", error_get_pretty(error));
> +        error_propagate(errp, error);
Why do it here but not above?

>          error_free(error);
>          goto error;
>      }
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
[...]
>  
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
> -    X86CPU *cpu;
> +    X86CPU *cpu = NULL;
>      Error *error = NULL;
>  
> -    cpu = cpu_x86_create(cpu_model);
> -    if (!cpu) {
> -        return NULL;
> +    cpu = cpu_x86_create(cpu_model, &error);
> +    if (error) {
> +        goto error;
>      }
>  
>      x86_cpu_realize(OBJECT(cpu), &error);
if x86_cpu_realize() behave as visit* functions, i.e. return early if
error has been already set, error check & goto could be removed here
and above and consolidated at function exit.

>      if (error) {
> -        error_free(error);
> -        object_delete(OBJECT(cpu));
> -        return NULL;
> +        goto error;
>      }
>      return cpu;
> +
> +error:
> +    if (cpu) {
> +        object_delete(OBJECT(cpu));
> +    }
> +    error_report("%s", error_get_pretty(error));
> +    error_free(error);
> +    return NULL;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.11.7
>
Eduardo Habkost - Dec. 14, 2012, 12:38 p.m.
On Fri, Dec 14, 2012 at 11:18:18AM +0100, Igor Mammedov wrote:
> On Wed, 12 Dec 2012 16:16:23 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Instead of forcing the caller to guess what went wrong while creating
> > the CPU object, return error information in a Error argument.
> > 
> > Also, as cpu_x86_create() won't print error messages itself anymore,
> > change cpu_x86_init() to print any error returned by cpu_x86_create()
> > or cpu_x86_realize().
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c    |  8 ++++++--
> >  target-i386/cpu.h    |  2 +-
> >  target-i386/helper.c | 21 ++++++++++++++-------
> >  3 files changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b242bf1..fba872d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1542,7 +1542,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
> >  /* Create and initialize a X86CPU object, based on the full CPU model string
> >   * (that may include "+feature,-feature,feature=xxx" feature strings)
> >   */
> > -X86CPU *cpu_x86_create(const char *cpu_model)
> > +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
> >  {
> >      X86CPU *cpu;
> >      CPUX86State *env;
> > @@ -1559,12 +1559,14 @@ X86CPU *cpu_x86_create(const char *cpu_model)
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> >      if (!model_pieces[0]) {
> > +        error_setg(errp, "invalid CPU model string: %s", cpu_model);
> >          goto error;
> >      }
> >      name = model_pieces[0];
> >      features = model_pieces[1];
> >  
> >      if (cpu_x86_find_by_name(def, name) < 0) {
> > +        error_setg(errp, "CPU model not found: %s", name);
> >          goto error;
> >      }
> >  
> > @@ -1575,13 +1577,15 @@ X86CPU *cpu_x86_create(const char *cpu_model)
> >                              &def->svm_features, &def->cpuid_7_0_ebx_features);
> >  
> >      if (cpu_x86_parse_featurestr(def, features) < 0) {
> > +        error_setg(errp, "Error parsing feature string: %s",
> > +                   features ? features : "(none)");
> It could be simplified, it shouldn't get here if features == NULL

It could be something like:

  if (features && cpu_x86_parse_featurestr(def, features) < 0) {
      ...
  }

Both options are reasonable to me.


> 
> >          goto error;
> >      }
> >  
> >      cpudef_2_x86_cpu(cpu, def, &error);
> >  
> >      if (error) {
> > -        fprintf(stderr, "%s\n", error_get_pretty(error));
> > +        error_propagate(errp, error);
> Why do it here but not above?

Because the other functions called above don't get an Error object
(yet).  :-)

> 
> >          error_free(error);
> >          goto error;
> >      }
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> [...]
> >  
> >  X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> > -    X86CPU *cpu;
> > +    X86CPU *cpu = NULL;
> >      Error *error = NULL;
> >  
> > -    cpu = cpu_x86_create(cpu_model);
> > -    if (!cpu) {
> > -        return NULL;
> > +    cpu = cpu_x86_create(cpu_model, &error);
> > +    if (error) {
> > +        goto error;
> >      }
> >  
> >      x86_cpu_realize(OBJECT(cpu), &error);
> if x86_cpu_realize() behave as visit* functions, i.e. return early if
> error has been already set, error check & goto could be removed here
> and above and consolidated at function exit.

I'm not sure I want to use that coding style. Expecting every function
to abort in the beginning if Error is set sounds fragile, to me. I would
even expect the maintainers to complain if I wrote the code that way (as
I never saw that style being used in any code except the visitors).

The visitors seem to be different because they are called from
automatically-generated QAPI code, that can't know if errors in a give
"visit" should abort the rest of the process, or not.


> 
> >      if (error) {
> > -        error_free(error);
> > -        object_delete(OBJECT(cpu));
> > -        return NULL;
> > +        goto error;
> >      }
> >      return cpu;
> > +
> > +error:
> > +    if (cpu) {
> > +        object_delete(OBJECT(cpu));
> > +    }
> > +    error_report("%s", error_get_pretty(error));
> > +    error_free(error);
> > +    return NULL;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -- 
> > 1.7.11.7
> > 
> 
> 
> -- 
> Regards,
>   Igor

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b242bf1..fba872d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1542,7 +1542,7 @@  static void filter_features_for_kvm(X86CPU *cpu)
 /* Create and initialize a X86CPU object, based on the full CPU model string
  * (that may include "+feature,-feature,feature=xxx" feature strings)
  */
-X86CPU *cpu_x86_create(const char *cpu_model)
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu;
     CPUX86State *env;
@@ -1559,12 +1559,14 @@  X86CPU *cpu_x86_create(const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
+        error_setg(errp, "invalid CPU model string: %s", cpu_model);
         goto error;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
+        error_setg(errp, "CPU model not found: %s", name);
         goto error;
     }
 
@@ -1575,13 +1577,15 @@  X86CPU *cpu_x86_create(const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
+        error_setg(errp, "Error parsing feature string: %s",
+                   features ? features : "(none)");
         goto error;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
     if (error) {
-        fprintf(stderr, "%s\n", error_get_pretty(error));
+        error_propagate(errp, error);
         error_free(error);
         goto error;
     }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 3ebaae9..8ce9b9f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -980,7 +980,7 @@  int cpu_x86_signal_handler(int host_signum, void *pinfo,
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
-X86CPU *cpu_x86_create(const char *cpu_model);
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 23af4a8..dafa666 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -23,6 +23,7 @@ 
 #include "sysemu.h"
 #include "monitor.h"
 #endif
+#include "qemu-error.h"
 
 //#define DEBUG_MMU
 
@@ -1242,21 +1243,27 @@  int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
 
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
-    X86CPU *cpu;
+    X86CPU *cpu = NULL;
     Error *error = NULL;
 
-    cpu = cpu_x86_create(cpu_model);
-    if (!cpu) {
-        return NULL;
+    cpu = cpu_x86_create(cpu_model, &error);
+    if (error) {
+        goto error;
     }
 
     x86_cpu_realize(OBJECT(cpu), &error);
     if (error) {
-        error_free(error);
-        object_delete(OBJECT(cpu));
-        return NULL;
+        goto error;
     }
     return cpu;
+
+error:
+    if (cpu) {
+        object_delete(OBJECT(cpu));
+    }
+    error_report("%s", error_get_pretty(error));
+    error_free(error);
+    return NULL;
 }
 
 #if !defined(CONFIG_USER_ONLY)