| Message ID | 1365172636-28628-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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.
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. >
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. >> >
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);
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(-)