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

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2014-03-03 at 11:50 +0100, Paolo Bonzini wrote:
> 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.
Paolo, I really appreciate you are taking the time to go over this!

> 
> 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.
I will, thanks.
> 
> The .machine field from QEMUMachineArgs can be removed very early.  It 
> can be replaced with the class of current_machine.
Sure.
> 
> Another early refactoring should be to pass &current_machine->init_args 
> to machine->init, not the "args".
Problem is that this is a "private field", should I add a getter for it?

> 
> 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.
I have nothing against it, but this will break QEMU's unified way to
handle object properties, right?
Meaning I will have a field  of the object state(kernel_filename)
and I will add a method like machine_get_field(QemuMachineState) going
"around" QOM,,, 

> 
> 2) Similarly, add get/set functions (not properties, since these are not 
> accessible via -machine) for ram_size, boot_order, cpu_model.
I am sorry, here you lost me, what do you mean "accessible via -machine"?
Maybe that cannot be queried by QOM tree?
Those getter/setters are not the same wrappers as above, going around "QOM"?


> 
> 3) Now you can have something like patch 8 in this series.
I also need patch 5 that deals with getting a string property with NULL value,
> 
> 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".
This will touch a lot of files... but needs to be done.
> 
> 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.
Agreed
> 
> Does it look feasible?  The bulk changes should all be fairly mechanical.
I see no reason why not, the main problem I see is the use of those wrappers
or setters/getters, I suspect that the usage will be:
1. global QOM query to get the machine
2. Use this wrappers(getters/setters) to do query/alter the machine.
Doesn't QOM have another way to do this? Or I am missing something, of course.

Thanks again for the review!
Marcel

> 
> Paolo
>