Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

On Wed, Jul 11, 2018 at 05:18:03PM +0200, Paolo Bonzini wrote:
> On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> > On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> >> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> >> +static void *qpci_get_driver(void *obj, const char *interface)
> >>  {
> >> -    QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> >> +    QPCIBusPC *qpci = obj;
> >> +    if (!g_strcmp0(interface, "pci-bus")) {
> >> +        return &qpci->bus;
> >> +    }
> >> +    printf("%s not present in pci-bus-pc", interface);
> >> +    abort();
> >> +}
> > 
> > At this point I wonder if it makes sense to use the QEMU Object Model
> > (QOM), which has interfaces and inheritance.  qgraph duplicates part of
> > the object model.
> 
> Replying for Emanuele on this point since we didn't really go through
> QOM yet; I'll let him answer the comments that are more related to the code.
> 
> QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
> 
> - QOM properties interact with QAPI and bring in a lot of baggage;
> qgraph would only use "child" properties to implement containment.
> 
> - QOM objects are long-lived, qgraph objects only last for the duration
> of a single test.  qgraph doesn't need reference counting or complex
> two-phase initialization like "realize" or "user_complete"
> 
> - QOM's hierarchy is shallow, but qgraph doesn't really need _any_
> hierarchy.  Because it focuses on interface rather than hierarchy,
> everything can be expressed simply through structs contained into one
> another.
> 
> Consider that qgraph is 1/4th the size of QOM, and a large part of it is
> the graph data structure that (as you said) would not be provided by QOM.
> 
> There are two things where using QOM would save a little bit of
> duplicated concepts, namely the get_driver (get interface, what you
> quote above) and get_device (get contained object) callbacks.  However,
> it wouldn't simplify the code at all, because it would require the
> introduction of separate instance and class structs.  We didn't want to
> add all too much boilerplate for people that want to write qtest, as you
> pointed out in the review of patch 4.

Yes, I think these are good points.  QOM does involve a lot of
boilerplate for defining objects.

> >> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> > 
> > It's not clear to me what the purpose of this function is - at least the
> > name is a bit cryptic since it seems more like an initialization
> > function than 'setting pc' on QPCIBusPC.  How about inlining this in
> > qpci_init_pc() instead of keeping a separate function?
> 
> I agree about the naming.  Perhaps we can rename the existing
> qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
> 
> Would you prefer if the split was done in the patch that introduces the
> user for qpci_set_pc (patch 5)?  We did it here because this patch
> prepares qpci-pc

Either way is fine.  I suggested inlining mainly because it avoids the
need to pick a good name :).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]