Re: [Qemu-devel] [PATCH v1 1/5] machine: Add a valid_cpu_types property

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Oct 03, 2017 at 01:26:53PM -0700, Alistair Francis wrote:
> On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote:
> >> This patch add a MachineClass element that can be set in the machine C
> >> code to specify a list of supported CPU types. If the supported CPU
> >> types are specified the user enter CPU (by -cpu at runtime) is checked
> >> against the supported types and QEMU exits if they aren't supported.
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> ---
> >
> > Thanks!
> >
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > However, I will squash the following changes before queueing,
> > because:
> >
> > * object_class_dynamic_cast() is safe even if class is NULL,
> >   so there's no need to validate cpu_type here.
> > * "must not be valid" sounds like the CPU is not allowed to be a
> >   valid CPU, so I rewrote the comment.
> >
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 3afc6a7b5b..36c2fb069c 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine)
> >          ObjectClass *class = object_class_by_name(machine->cpu_type);
> >          int i;
> >
> > -        /* machine->cpu_type is supposed to be always a valid QOM type */
> > -        assert(class);
> > -
> >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> >              if (object_class_dynamic_cast(class,
> >                                            machine_class->valid_cpu_types[i])) {
> > @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine)
> >          }
> >
> >          if (!machine_class->valid_cpu_types[i]) {
> > -            /* The user specified CPU must not be a valid CPU */
> > +            /* The user specified CPU is not valid */
> >              error_report("Invalid CPU type: %s", machine->cpu_type);
> >              error_printf("The valid types are: %s",
> >                           machine_class->valid_cpu_types[0]);
> 
> Looks good to me.
> 
> Does that mean you are taking the whole series now?

I was planning to tacke only patch 1/5, but I can take the whole
series if I get an Acked-by from the corresponding maintainers.

-- 
Eduardo