Message ID | 20190417025944.16154-3-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
Series | Remove qdev_get_machine() call from ppc_cpu_parse_featurestr() | expand |
On Tue, Apr 16, 2019 at 11:59:41PM -0300, Eduardo Habkost wrote: > The new function will be useful in user mode, when we already > have a CPU model and don't need to parse any extra options. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > include/qom/cpu.h | 9 +++++++++ > exec.c | 22 ++++++++++++---------- > 2 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index d28c690b27..e11b14d9ac 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -699,6 +699,15 @@ CPUState *cpu_create(const char *typename); > */ > const char *parse_cpu_option(const char *cpu_option); > > +/** > + * lookup_cpu_class: > + * @cpu_model: CPU model name > + * > + * Look up CPU class corresponding to a given CPU model name. > + */ > +CPUClass *lookup_cpu_class(const char *cpu_model, Error **errp); > + > + > /** > * cpu_has_work: > * @cpu: The vCPU to check. > diff --git a/exec.c b/exec.c > index 840677f15f..d359e709a6 100644 > --- a/exec.c > +++ b/exec.c > @@ -982,24 +982,26 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) > #endif > } > > +CPUClass *lookup_cpu_class(const char *cpu_model, Error **errp) > +{ > + ObjectClass *oc = cpu_class_by_name(CPU_RESOLVING_TYPE, cpu_model); > + if (oc == NULL) { > + error_setg(errp, "unable to find CPU model '%s'", cpu_model); > + return NULL; > + } > + return CPU_CLASS(oc); > +} > + > const char *parse_cpu_option(const char *cpu_option) > { > - ObjectClass *oc; > CPUClass *cc; > gchar **model_pieces; > const char *cpu_type; > > model_pieces = g_strsplit(cpu_option, ",", 2); > > - oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]); > - if (oc == NULL) { > - error_report("unable to find CPU model '%s'", model_pieces[0]); > - g_strfreev(model_pieces); > - exit(EXIT_FAILURE); > - } > - > - cpu_type = object_class_get_name(oc); > - cc = CPU_CLASS(oc); > + cc = lookup_cpu_class(model_pieces[0], &error_fatal); > + cpu_type = object_class_get_name(OBJECT_CLASS(cc)); > cc->parse_features(cpu_type, model_pieces[1], &error_fatal); > g_strfreev(model_pieces); > return cpu_type;
Eduardo Habkost <ehabkost@redhat.com> writes: > The new function will be useful in user mode, when we already > have a CPU model and don't need to parse any extra options. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > include/qom/cpu.h | 9 +++++++++ > exec.c | 22 ++++++++++++---------- > 2 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index d28c690b27..e11b14d9ac 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -699,6 +699,15 @@ CPUState *cpu_create(const char *typename); > */ > const char *parse_cpu_option(const char *cpu_option); > > +/** > + * lookup_cpu_class: > + * @cpu_model: CPU model name > + * > + * Look up CPU class corresponding to a given CPU model name. > + */ > +CPUClass *lookup_cpu_class(const char *cpu_model, Error **errp); Nitpicks, feel free to ignore. Naming: lookup_cpu_class() makes my reading circuits stall momentarily, because lookup is a noun. The verb is spelled look up. No stall: cpu_class_lookup(), look_up_cpu_class(), cpu_class_by_name_err(), cpu_class_parse(), ... Doc string: this is a wrapper around cpu_class_by_name(). Perhaps that should be spelled out. Apropos cpu_class_by_name(): it returns ObjectClass, not CPUClass. Isn't that odd? Position: If you put it before cpu_create() rather than after, it's next to the function it wraps. But perhaps you prefer to keep it next to parse_cpu_option(). [...]
On Wed, Apr 17, 2019 at 07:41:23AM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > The new function will be useful in user mode, when we already > > have a CPU model and don't need to parse any extra options. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > include/qom/cpu.h | 9 +++++++++ > > exec.c | 22 ++++++++++++---------- > > 2 files changed, 21 insertions(+), 10 deletions(-) > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index d28c690b27..e11b14d9ac 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -699,6 +699,15 @@ CPUState *cpu_create(const char *typename); > > */ > > const char *parse_cpu_option(const char *cpu_option); > > > > +/** > > + * lookup_cpu_class: > > + * @cpu_model: CPU model name > > + * > > + * Look up CPU class corresponding to a given CPU model name. > > + */ > > +CPUClass *lookup_cpu_class(const char *cpu_model, Error **errp); > > Nitpicks, feel free to ignore. > > Naming: lookup_cpu_class() makes my reading circuits stall momentarily, > because lookup is a noun. The verb is spelled look up. No stall: > cpu_class_lookup(), look_up_cpu_class(), cpu_class_by_name_err(), > cpu_class_parse(), ... > > Doc string: this is a wrapper around cpu_class_by_name(). Perhaps that > should be spelled out. > > Apropos cpu_class_by_name(): it returns ObjectClass, not CPUClass. > Isn't that odd? > > Position: If you put it before cpu_create() rather than after, it's next > to the function it wraps. But perhaps you prefer to keep it next to > parse_cpu_option(). Your suggestions make sense. I didn't notice how close lookup_cpu_class() was to the declaration of cpu_class_by_name(). I was seeing lookup_cpu_class() as "this portion of parse_cpu_option() I need", and not as "a wrapper to cpu_class_by_name()". What about: /** * arch_cpu_class_by_name: * @cpu_model: CPU model name * * Arch-specific wrapper around cpu_class_by_name(), uses CPU_RESOLVING_TYPE * for the current architecture. */ CPUClass *arch_cpu_class_by_name(const char *cpu_model, Error **errp);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h index d28c690b27..e11b14d9ac 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -699,6 +699,15 @@ CPUState *cpu_create(const char *typename); */ const char *parse_cpu_option(const char *cpu_option); +/** + * lookup_cpu_class: + * @cpu_model: CPU model name + * + * Look up CPU class corresponding to a given CPU model name. + */ +CPUClass *lookup_cpu_class(const char *cpu_model, Error **errp); + + /** * cpu_has_work: * @cpu: The vCPU to check. diff --git a/exec.c b/exec.c index 840677f15f..d359e709a6 100644 --- a/exec.c +++ b/exec.c @@ -982,24 +982,26 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) #endif } +CPUClass *lookup_cpu_class(const char *cpu_model, Error **errp) +{ + ObjectClass *oc = cpu_class_by_name(CPU_RESOLVING_TYPE, cpu_model); + if (oc == NULL) { + error_setg(errp, "unable to find CPU model '%s'", cpu_model); + return NULL; + } + return CPU_CLASS(oc); +} + const char *parse_cpu_option(const char *cpu_option) { - ObjectClass *oc; CPUClass *cc; gchar **model_pieces; const char *cpu_type; model_pieces = g_strsplit(cpu_option, ",", 2); - oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]); - if (oc == NULL) { - error_report("unable to find CPU model '%s'", model_pieces[0]); - g_strfreev(model_pieces); - exit(EXIT_FAILURE); - } - - cpu_type = object_class_get_name(oc); - cc = CPU_CLASS(oc); + cc = lookup_cpu_class(model_pieces[0], &error_fatal); + cpu_type = object_class_get_name(OBJECT_CLASS(cc)); cc->parse_features(cpu_type, model_pieces[1], &error_fatal); g_strfreev(model_pieces); return cpu_type;
The new function will be useful in user mode, when we already have a CPU model and don't need to parse any extra options. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- include/qom/cpu.h | 9 +++++++++ exec.c | 22 ++++++++++++---------- 2 files changed, 21 insertions(+), 10 deletions(-)