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

[Eduardo Habkost]

On Tue, Sep 05, 2017 at 07:41:51AM +0200, Thomas Huth 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 <address@hidden>
> > ---
> > 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...

I would prefer that, but note that in that case
cpu_parse_features() shouldn't leave any global property
registered if an error is reported.  It would need to validate
all the input before registering any properties, or rollback the
global properties it already registered when reporting an error.

-- 
Eduardo