Re: [Qemu-devel] [RFC 20/42] machine: add cpu-hotplug machine option

[Eduardo Habkost]

On Wed, May 11, 2016 at 04:00:19PM +0200, Igor Mammedov wrote:
> On Tue, 10 May 2016 17:27:18 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <address@hidden>  
> > 
> > If a machine class simply doesn't support CPU hotplug at all, is
> > silently ignoring "cpu-hotplug=on" the right thing to do?
> > 
> > Shouldn't we exit with an error if the machine class doesn't
> > support CPU hotplug and the user tries to enable it?
> We have a bunch of such options in generic MachineClass
> and it was upto concrete machine to implement such checks.

Yes, and my proposal is to make this more robust from now on, and
make machines stop silently ignoring unsupported options.

> 
> So I hadn't even considered to make such check in generic code
> nor think that's a right thing to do, if that's what you are asking.

If only 1 or 2 machines support CPU hotplug, I don't think it
would be reasonable to have the option available at every machine
class and require most classes to add an extra check like:

    if (machine->cpu_hotplug)
        error_setg(errp, "CPU hotplug not supported");


A check like the following, in common code:

    if (!machine_class->cpu_hotplug_supported && machine->cpu_hotplug)
        error_setg(errp, "CPU hotplug not supported")

would avoid duplicating code in every machine class, and only
require the following extra line in the machine classes that
really support CPU hotplug:

    machine_class->cpu_hotplug_supported = true;


Or, even better: we can avoid registering the cpu-hotplug
property at all if the subclass doesn't support CPU hotplug. You
can do that at machine_class_base_init, e.g.:

    static void machine_class_base_init(ObjectClass *oc, void *data)
    {
        /* [...] */
        MachineClass *mc = MACHINE_CLASS(oc);
        if (mc->cpu_hotplug_supported) {
            object_class_property_add_str(oc, "cpu-hotplug",
                                          machine_get_cpu_hotplug,
                                          machine_set_cpu_hotplug, NULL);
            object_class_property_set_description(oc, "cpu-hotplug",
                                                  "Set on to enable CPU 
hotplug",
                                                  NULL);
        }
    }


-- 
Eduardo