[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface n
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes |
Date: |
Wed, 18 Jul 2018 15:29:27 +0100 |
User-agent: |
Mutt/1.10.0 (2018-05-17) |
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
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes, Philippe Mathieu-Daudé, 2018/07/11
[Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci driver and interface nodes, Emanuele Giuseppe Esposito, 2018/07/09