On Fri, Dec 14, 2012 at 11:18:18AM +0100, Igor Mammedov wrote:
> 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
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
--
Eduardo
