Marcel Apfelbaum <marce...@redhat.com> writes: > On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote: >> John Snow <js...@redhat.com> writes: >> >> > Signed-off-by: John Snow <js...@redhat.com> [...] >> > @@ -1583,6 +1584,7 @@ static void machine_class_init(ObjectClass *oc, void >> > *data) >> > mc->hot_add_cpu = qm->hot_add_cpu; >> > mc->kvm_type = qm->kvm_type; >> > mc->block_default_type = qm->block_default_type; >> > + mc->units_per_idebus = qm->units_per_idebus; >> > mc->max_cpus = qm->max_cpus; >> > mc->no_serial = qm->no_serial; >> > mc->no_parallel = qm->no_parallel; >> >> Not sufficient! You missed the duplicated code in >> pc_generic_machine_class_init(). That something was missing was quite >> obvious in my testing, although what was missing was not. This is the >> fix I mentioned above. >> >> Marcel, you touched both copies recently. Do you know why we need two >> of them? And why do we copy from QEMUMachine to MachineClass member by >> member? Why can't we just point from MachineClass to QEMUMachine? Or >> at least embed the struct so we can copy it wholesale? > Hi Markus, > > I'll try to explain the design: > 1. MachineClass should not be aware of QEMUMachine. The objective here is to > eliminate QEMUMachine and it should not be part of MachineClass at any > cost. > 2. The plan is like this: > - The machine types that are not yet QOMified will be QOMified on the fly > by qemu_register_machine (vl.c) that calls machine_class_init and matches > QEMUMachine fields with MachineClass fields. > - This mechanism will be removed when all machines are QOMified. (never? > :) )
Okay %-) > - Machines that are QOMified will not reach this code at all, but follow > the normal QOM path. > As you can see, by design there is no duplication. > > Now let's get to PC machines case: > - This is a "weird" use case of hybrid QOMifying. > - All that the author wanted was to have all the PC machines > derive from type TYPE_PC_MACHINE, but didn't have the time/resources/will > to go over every PC machine and QOMify it. But he did need the common class. > - His implementation: > - He couldn't use the generic qemu_register_machine because the PC machines > would not have derived from MACHINE_PC_TYPE. > - So he used the following logic: > - from the vl.c point of view, the PC machines are QOMified, so the > PC machines registration *does not reach vl.c". > - from the PC machines point of view, they remained not QOMified. > - qemu_register_pc_machine mimics vl.c registration *only for pc machines* > and has to copy the fields by itself. Plus, it gives the PC machines a > common > base class, the thing he was interested in in the first place. Artifact of this hackery: two identical static functions: vl.c's machine_class_init() and pc.c's pc_generic_machine_class_init(). Trap for the unwary, and it caught John. Unless there are plans to get rid of pc_generic_machine_class_init() fairly soon, I'd recommend to give machine_class_init() external linkage with a suitable comment, and drop pc_generic_machine_class_init(). > I hope it helped, Sure did! I checked the CodeTransitions Wiki page. It covers this work, and points to http://wiki.qemu.org/Features/QOM/Machine for more information. Good.
