Re: Suspicious QOM types without instance/class size

[Eduardo Habkost]

On Fri, Aug 21, 2020 at 01:29:38PM -0400, Eduardo Habkost wrote:
> On Fri, Aug 21, 2020 at 01:53:52PM +0300, Roman Bolshakov wrote:
> > On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> > > While trying to convert TypeInfo declarations to the new
> > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> > > where instance_size or class_size is not set, despite having type
> > > checker macros that use a specific type.
> > > 
> > > The ones with "WARNING" are abstract types (maybe not serious if
> > > subclasses set the appropriate sizes).  The ones with "ERROR"
> > > don't seem to be abstract types.
> > > 
> > 
> > > ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to 
> > > sizeof(HVFState)?
> > 
> > Hi Eduardo,
> > 
> > How do you get the error?
> 
> My script looks for corresponding type checking macros, and check
> if instance_size is set to sizeof(T) with the right type from the
> type checking macro.
> 
> The code is here:
> https://github.com/ehabkost/qemu-hacks/blob/920b2c521ad2a29fa663256854e24ed2059ba9cd/scripts/codeconverter/codeconverter/qom_type_info.py#L136
> 
> 
> > 
> > Given your changes, instance size should really be sizeof(HVFState).
> > 
> 
> The changes I've made shouldn't make any difference (if there's
> an issue, it is there before or after my series).
> 
> > BTW, the object definition for hvf seems different from KVM (and perhaps
> > wrong?), e.g. HVFState is allocated within init_machine handler and then
> > assigned to a global variable:
> 
> Interesting.  It looks like hvf_state is _not_ the actual QOM
> object instance.  The actual TYPE_HVF_ACCEL instance is created
> by do_configure_accelerator().  That would explain why the lack
> of instance_init never caused any problems.
> 
> Luckily, no code ever used the HVF_STATE macro.  If
> HVF_STATE(hvf_state) got called, it would crash because of
> uninitialized object instance data.  If HVF_STATE(machine->accel)
> got called, it would return an invalid HVFState pointer (not
> hvf_state).
> 
> I believe the simplest short term solution here is to just delete
> the HVF_STATE macro and HVFState::parent field.  We can worry
> about actually moving hvf_state to the machine->accel QOM object
> later.

Actually, it might be easier to do the full QOM conversion in a
single patch instead of deleting the incomplete code.

Can you check if the following patch works?  I don't have a host
where I can test it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index d81f569aed..81d1662d06 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -878,13 +878,11 @@ static int hvf_accel_init(MachineState *ms)
 {
     int x;
     hv_return_t ret;
-    HVFState *s;
+    HVFState *s = HVF_STATE(ms->accelerator);
 
     ret = hv_vm_create(HV_VM_DEFAULT);
     assert_hvf_ok(ret);
 
-    s = g_new0(HVFState, 1);
- 
     s->num_slots = 32;
     for (x = 0; x < s->num_slots; ++x) {
         s->slots[x].size = 0;
@@ -908,6 +906,7 @@ static void hvf_accel_class_init(ObjectClass *oc, void 
*data)
 static const TypeInfo hvf_accel_type = {
     .name = TYPE_HVF_ACCEL,
     .parent = TYPE_ACCEL,
+    .instance_size = sizeof(HVFState),
     .class_init = hvf_accel_class_init,
 };
 
 
-- 
Eduardo