Patchwork [03/22] target-i386: split out CPU creation and features parsing into cpu_x86_create()

login
register
mail settings
Submitter Igor Mammedov
Date April 5, 2013, 2:36 p.m.
Message ID <1365172636-28628-4-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/234153/
State New
Headers show

Comments

Igor Mammedov - April 5, 2013, 2:36 p.m.
Move CPU creation and features parsing into a separate cpu_x86_create()
function, so that board would be able to set board specific CPU
properties before CPU is realized.

Keep cpu_x86_init() for compatibility with the code that uses cpu_init()
and doesn't need to modify CPU properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 28 +++++++++++++++++++---------
 target-i386/cpu.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)
Eduardo Habkost - April 8, 2013, 6:30 p.m.
On Fri, Apr 05, 2013 at 04:36:55PM +0200, Igor Mammedov wrote:
> Move CPU creation and features parsing into a separate cpu_x86_create()
> function, so that board would be able to set board specific CPU
> properties before CPU is realized.
> 
> Keep cpu_x86_init() for compatibility with the code that uses cpu_init()
> and doesn't need to modify CPU properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 28 +++++++++++++++++++---------
>  target-i386/cpu.h |  1 +
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 41f0f47..269a681 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1563,17 +1563,16 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
>  }
>  
> -X86CPU *cpu_x86_init(const char *cpu_model)
> +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>  {
>      X86CPU *cpu = NULL;
>      CPUX86State *env;
>      gchar **model_pieces;
>      char *name, *features;
> -    Error *error = NULL;
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> -        error_setg(&error, "Invalid/empty CPU model name");
> +        error_setg(errp, "Invalid/empty CPU model name");
>          goto out;
>      }
>      name = model_pieces[0];
> @@ -1583,23 +1582,34 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      env = &cpu->env;
>      env->cpu_model_str = cpu_model;
>  
> -    cpu_x86_register(cpu, name, &error);
> -    if (error) {
> +    cpu_x86_register(cpu, name, errp);
> +    if (error_is_set(errp)) {

So the function now does error checking properly if and only if errp is
not NULL. Do we really want to do that?

>          goto out;
>      }
>  
> -    cpu_x86_parse_featurestr(cpu, features, &error);
> -    if (error) {
> +    cpu_x86_parse_featurestr(cpu, features, errp);
> +    if (error_is_set(errp)) {
>          goto out;
>      }
>  
> -    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
> +out:
> +    g_strfreev(model_pieces);

Any specific reason you didn't choose to keep 'Error *error = NULL'
inside cpu_x86_create() as well, and use error_propagate() here? I
believe it would make the patch simpler and easier to review, and at the
same time make cpu_x86_init() check for errors properly even if errp is
NULL. This is the opposite of what you did on x86_cpu_realizefn() at
patch 01/22.

I am not against it if you want to keep this style of error-checking,
but I believe an error_propagate()-based version would be simpler and
safer.


> +    return cpu;
> +}
> +
> +X86CPU *cpu_x86_init(const char *cpu_model)
> +{
> +    Error *error = NULL;
> +    X86CPU *cpu;
> +
> +    cpu = cpu_x86_create(cpu_model, &error);
>      if (error) {
>          goto out;
>      }
>  
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
> +
>  out:
> -    g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 069a2e2..b98efd2 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -896,6 +896,7 @@ typedef struct CPUX86State {
>  #include "cpu-qom.h"
>  
>  X86CPU *cpu_x86_init(const char *cpu_model);
> +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
>  int cpu_x86_exec(CPUX86State *s);
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>  void x86_cpudef_setup(void);
> -- 
> 1.8.1.4
> 
>
Paolo Bonzini - April 9, 2013, 10:30 a.m.
Il 08/04/2013 20:30, Eduardo Habkost ha scritto:
>> > -    cpu_x86_register(cpu, name, &error);
>> > -    if (error) {
>> > +    cpu_x86_register(cpu, name, errp);
>> > +    if (error_is_set(errp)) {
> So the function now does error checking properly if and only if errp is
> not NULL. Do we really want to do that?

No, using error_propagate is the correct idiom indeed.

Paolo

>> >          goto out;
>> >      }
>> >  
>> > -    cpu_x86_parse_featurestr(cpu, features, &error);
>> > -    if (error) {
>> > +    cpu_x86_parse_featurestr(cpu, features, errp);
>> > +    if (error_is_set(errp)) {
>> >          goto out;
>> >      }
>> >  
>> > -    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
>> > +out:
>> > +    g_strfreev(model_pieces);
> Any specific reason you didn't choose to keep 'Error *error = NULL'
> inside cpu_x86_create() as well, and use error_propagate() here? I
> believe it would make the patch simpler and easier to review, and at the
> same time make cpu_x86_init() check for errors properly even if errp is
> NULL. This is the opposite of what you did on x86_cpu_realizefn() at
> patch 01/22.
Igor Mammedov - April 9, 2013, 10:33 a.m.
On Tue, 09 Apr 2013 12:30:21 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 08/04/2013 20:30, Eduardo Habkost ha scritto:
> >> > -    cpu_x86_register(cpu, name, &error);
> >> > -    if (error) {
> >> > +    cpu_x86_register(cpu, name, errp);
> >> > +    if (error_is_set(errp)) {
> > So the function now does error checking properly if and only if errp is
> > not NULL. Do we really want to do that?
> 
> No, using error_propagate is the correct idiom indeed.
Ok, I'll use  error_propagate() in next version.

> 
> Paolo
> 
> >> >          goto out;
> >> >      }
> >> >  
> >> > -    cpu_x86_parse_featurestr(cpu, features, &error);
> >> > -    if (error) {
> >> > +    cpu_x86_parse_featurestr(cpu, features, errp);
> >> > +    if (error_is_set(errp)) {
> >> >          goto out;
> >> >      }
> >> >  
> >> > -    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
> >> > +out:
> >> > +    g_strfreev(model_pieces);
> > Any specific reason you didn't choose to keep 'Error *error = NULL'
> > inside cpu_x86_create() as well, and use error_propagate() here? I
> > believe it would make the patch simpler and easier to review, and at the
> > same time make cpu_x86_init() check for errors properly even if errp is
> > NULL. This is the opposite of what you did on x86_cpu_realizefn() at
> > patch 01/22.
>
Andreas Färber - April 9, 2013, 2:02 p.m.
Am 09.04.2013 12:33, schrieb Igor Mammedov:
> On Tue, 09 Apr 2013 12:30:21 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 08/04/2013 20:30, Eduardo Habkost ha scritto:
>>>>> -    cpu_x86_register(cpu, name, &error);
>>>>> -    if (error) {
>>>>> +    cpu_x86_register(cpu, name, errp);
>>>>> +    if (error_is_set(errp)) {
>>> So the function now does error checking properly if and only if errp is
>>> not NULL. Do we really want to do that?
>>
>> No, using error_propagate is the correct idiom indeed.
> Ok, I'll use  error_propagate() in next version.

I remember accepting some CPU refactoring patch but asking you to fix up
the same issue as follow-up - I don't remember having received a single
fix-up patch, could you please check on whether that is hidden in some
series or still missing? Thanks!

Andreas

> 
>>
>> Paolo
>>
>>>>>          goto out;
>>>>>      }
>>>>>  
>>>>> -    cpu_x86_parse_featurestr(cpu, features, &error);
>>>>> -    if (error) {
>>>>> +    cpu_x86_parse_featurestr(cpu, features, errp);
>>>>> +    if (error_is_set(errp)) {
>>>>>          goto out;
>>>>>      }
>>>>>  
>>>>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
>>>>> +out:
>>>>> +    g_strfreev(model_pieces);
>>> Any specific reason you didn't choose to keep 'Error *error = NULL'
>>> inside cpu_x86_create() as well, and use error_propagate() here? I
>>> believe it would make the patch simpler and easier to review, and at the
>>> same time make cpu_x86_init() check for errors properly even if errp is
>>> NULL. This is the opposite of what you did on x86_cpu_realizefn() at
>>> patch 01/22.
>>
>

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 41f0f47..269a681 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1563,17 +1563,16 @@  static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu = NULL;
     CPUX86State *env;
     gchar **model_pieces;
     char *name, *features;
-    Error *error = NULL;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        error_setg(&error, "Invalid/empty CPU model name");
+        error_setg(errp, "Invalid/empty CPU model name");
         goto out;
     }
     name = model_pieces[0];
@@ -1583,23 +1582,34 @@  X86CPU *cpu_x86_init(const char *cpu_model)
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    cpu_x86_register(cpu, name, &error);
-    if (error) {
+    cpu_x86_register(cpu, name, errp);
+    if (error_is_set(errp)) {
         goto out;
     }
 
-    cpu_x86_parse_featurestr(cpu, features, &error);
-    if (error) {
+    cpu_x86_parse_featurestr(cpu, features, errp);
+    if (error_is_set(errp)) {
         goto out;
     }
 
-    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
+out:
+    g_strfreev(model_pieces);
+    return cpu;
+}
+
+X86CPU *cpu_x86_init(const char *cpu_model)
+{
+    Error *error = NULL;
+    X86CPU *cpu;
+
+    cpu = cpu_x86_create(cpu_model, &error);
     if (error) {
         goto out;
     }
 
+    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
+
 out:
-    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 069a2e2..b98efd2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -896,6 +896,7 @@  typedef struct CPUX86State {
 #include "cpu-qom.h"
 
 X86CPU *cpu_x86_init(const char *cpu_model);
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 void x86_cpudef_setup(void);