Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Paolo Bonzini <pbonzini@redhat.com>]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> Most of the "Cc" list is due to patch 8: (Should I send each patch to a different list?)
>     machine-opts: replace qemu_opt_get by QOM QemuMachine queries.
>
> Status:
>     - machine_opts are mapped into QemuMachineState's properties,
>       which can be queried as regular QOM properties.
>     - Subclassing QemuMachineClass allows to add a command line
>       option specific to a machine type, error mechanism should
>       work if this option is used on another machine. (Not tested, on the todo list.)
>     - Next big step would be to completely remove the qemu machines initialization
>       and replace it by regular QOM type registration.
Having seen all the series, I think we can plan the actual code as follows.

Patches 1-3 are fine (except for the missing object_property_add_child 
in patch 3).  Patch 4 is also fine, but it should refer to the embedded 
QEMUMachineInitArgs.

The .machine field from QEMUMachineArgs can be removed very early.  It 
can be replaced with the class of current_machine.

Another early refactoring should be to pass &current_machine->init_args 
to machine->init, not the "args".

Now here's a possible plan to get rid of QEMUMachineInitArgs:

1) Add three wrappers to QemuMachine for reading kernel_filename, 
kernel_cmdline, initrd_filename.  Unlike object_property_get_str, they 
can skip the strdup of the value.  This way you don't have to add the 
matching free to all uses of the fields.

2) Similarly, add get/set functions (not properties, since these are not 
accessible via -machine) for ram_size, boot_order, cpu_model.

3) Now you can have something like patch 8 in this series.

4) Thanks to the previous change, there should be no usage of 
QEMUMachineInitArgs anymore.  Change the machine init function to take a 
"QemuMachineState *current_machine".

5) Remove the current_machine global.  There will be few users of 
current_machine, and they can look at /machine instead, as mentioned in 
the review of patch 3.

Does it look feasible?  The bulk changes should all be fairly mechanical.

Paolo