| Message ID | 1355336183-18858-3-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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)
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(-)