Re: [Qemu-devel] [PATCH 1/3] machine: remove qemu_machine_opts global list

[Alexander Graf <agraf@suse.de>]

On 11.12.14 06:05, Marcel Apfelbaum wrote:
> On Wed, 2014-12-10 at 16:59 -0600, Greg Bellows wrote:
>>
>>
>> On 10 December 2014 at 07:19, Marcel Apfelbaum <marcel.a@redhat.com>
>> wrote:
>>         QEMU has support for options per machine, keeping
>>         a global list of options is no longer necessary.
>>         
>>         Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>         ---
>>          hw/core/machine.c | 45 +++++++++++++++++++++++++++++
>>          hw/i386/pc.c      |  7 +++++
>>          hw/ppc/spapr.c    |  3 ++
>>          vl.c              | 84
>>         ++++---------------------------------------------------
>>          4 files changed, 61 insertions(+), 78 deletions(-)
>>         
>>         diff --git a/hw/core/machine.c b/hw/core/machine.c
>>         index 19d3e3a..a0ae5f9 100644
>>         --- a/hw/core/machine.c
>>         +++ b/hw/core/machine.c
>>         @@ -291,48 +291,93 @@ static void machine_initfn(Object *obj)
>>         
>>              object_property_add_str(obj, "accel",
>>                                      machine_get_accel,
>>         machine_set_accel, NULL);
>>         +    object_property_set_description(obj, "accel",
>>         +                                    "Accelerator list",
>>         +                                    NULL);
>>
> Hi Greg,
> Thank you for your review!
> 
>>
>> If this is a common combination of calls would it make sense to create
>> a wrapper routine that just does both of these steps together for
>> readability and convenience? Maybe something like:
>>
>>
>>         object_property_add_str_with_desc(obj, "accel", "Accelerator
>>         list"
>>                                      machine_get_accel,
>>         machine_set_accel,
>>                                      NULL);
> I really thought about it, but one wrapper would not be enough,
> we need also one for object_property_add_bool, object_property_add and so on.
> The code needed would be twice as this one and for the moment this
> series is the only reason for that, maybe is not enough.
> 
> If you or anyone else things we should still go for it, I'll be glad to.

Maybe something like this?

#define MACHINE_OPT(name, namestr, desc, type, ...) \
    glue(object_property_add_, type)(obj, namestr, \
        glue(machine_get, name), glue(machine_set, name), __VA_ARGS__); \
    \
    object_property_set_description(obj, namestr, desc, NULL);

> 
>>  
>>              object_property_add_bool(obj, "kernel-irqchip",
>>                                       machine_get_kernel_irqchip,
>>                                       machine_set_kernel_irqchip,
>>                                       NULL);
>>         +    object_property_set_description(obj, "kernel-irqchip",
>>         +                                    "Use KVM in-kernel
>>         irqchip",
>>         +                                    NULL);


MACHINE_OPT(kvm_shadow_mem, "kvm-shadow-mem", "Use KVM in-kernel
irqchip", _bool, NULL);

Not sure it's a great improvement though. I certainly wouldn't mind to
leave it as this patch does it.


Alex