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
signature.asc
Description: PGP signature
