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 ¤t_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 >
