Re: [Qemu-devel] [PATCH 2/6] cpu: make cpu_generic_init() abort QEMU on error

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 5 Sep 2017 07:41:51 +0200
Thomas Huth <huth@tuxfamily.org> wrote:

> On 04.09.2017 16:00, Igor Mammedov wrote:
> > Almost every user of cpu_generic_init() checks for
> > returned NULL and then reports failure in a custom way
> > and aborts process.
> > Some users assume that call can't fail and don't check
> > for failure, though they should have checked for it.
> > 
> > In either cases cpu_generic_init() failure is fatal,
> > so instead of checking for failure and reporting
> > it various ways, make cpu_generic_init() report
> > errors in consistent way and terminate QEMU on failure.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > Even though it's tree wide change, it's trivial so all
> > affected call sites are included within one patch.  
> [...]
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index d715890..307d638 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -61,7 +61,7 @@ CPUState *cpu_create(const char *typename)
> >      if (err != NULL) {
> >          error_report_err(err);
> >          object_unref(OBJECT(cpu));
> > -        return NULL;
> > +        exit(EXIT_FAILURE);
> >      }
> >      return cpu;
> >  }
> > @@ -78,8 +78,9 @@ const char *cpu_parse_features(const char *typename, const char *cpu_model)
> >  
> >      oc = cpu_class_by_name(typename, model_pieces[0]);
> >      if (oc == NULL) {
> > +        error_report("unable to find CPU model '%s'", model_pieces[0]);
> >          g_strfreev(model_pieces);
> > -        return NULL;
> > +        exit(EXIT_FAILURE);
> >      }
> >  
> >      cpu_type = object_class_get_name(oc);
> > @@ -88,7 +89,7 @@ const char *cpu_parse_features(const char *typename, const char *cpu_model)
> >      g_strfreev(model_pieces);
> >      if (err != NULL) {
> >          error_report_err(err);
> > -        return NULL;
> > +        exit(EXIT_FAILURE);
> >      }
> >      return cpu_type;
> >  }
> > @@ -100,10 +101,8 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
> >       */
> >      const char *cpu_type = cpu_parse_features(typename, cpu_model);
> >  
> > -    if (cpu_type) {
> > -        return cpu_create(cpu_type);
> > -    }
> > -    return NULL;
> > +    assert(cpu_type);
> > +    return cpu_create(cpu_type);
> >  }  
> 
> Not sure, but wouldn't it be better to do the error reporting and exit
> in cpu_generic_init() instead? In case we ever might want to re-use the
> create and parse_feature functions for device_add later (?), and then
> the functions must not exit directly anymore...
1st:
cpu_generic_init() should be removed once I convert all users to use
MachineState::cpu_type + cpu_create() instead of cpu_generic_init(cpu_model).
So no board would have to deal with cpu_model directly.

2nd:
device_add already does
  obj = object_new() + ... + obj.realize = true
hence cpu_create() is of no use there.
However cpu_create() is still useful as all users that
just need to create and realize cpu without extra configuration,
could benefit from unified error checking/message and 'small' code
deduplication as diffstat for this patch shows.

   38 files changed, 8 insertions(+), 169 deletions(-)

3rd:
parse_feature callbacks are meant to be called only once
and do nothing if called multiple times. They convert
'-cpu foo,features' option into a set of global properties which
are applied every created cpu instance of given type.
So when 'device_add cpu_foo' is called, there is no need to call
parse_features(). 


> 
>  Thomas
>