Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>> 
>> > Signed-off-by: John Snow <address@hidden>
[...]
>> > @@ -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.