On Wed, 12 Dec 2012 16:16:23 -0200
Eduardo Habkost <ehabk...@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 <ehabk...@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
>
--
Regards,
Igor
