[2/5] cpu: Extract CPU class lookup from parse_cpu_option()

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(-)

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);