On Tue, Feb 11, 2025 at 11:25:05AM +0100, Victor Toso wrote: > Hi, > > On Thu, Jan 16, 2025 at 09:59:52PM +0000, Daniel P. Berrangé wrote: > > <<<< Pause here if you've read enough for now. >>>> > > > > > > > > As a way to validate these thoughts, I spent a day to mock up a demo > > of a QAPI client and server implementation. > > > > First I created some manually written structs for the core QMP types > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/protocol.go > > > > Then I've got (what would be) generated code creating types for the > > specific QAPI schema. I've just illustrated this with query-machines > > and qmp_capabilities commands, and STOP, CONT, BALLOON_CHANGE events: > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_types.go > > > > Essentially this is all the types you generated, but without any of > > the (un)marshalling method impls which IMHO are all redundant. I put > > everything in one file just for convenience, your per-file split makes > > more sense in a real impl. > > > > > > IMHO that's all that's needed to cover the scope of what is done in > > this series, but to illustrate a real world usage model, I've then > > gone on to implement both clients and servers. > > > > At the very lowest level, both clients & servers need a way to send > > and recv "Message" instances, so I created a "Channel" object with > > SendMessage/RecvMessage methods: > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/channel.go > > > > This is the most basic low level client one could have for QMP > > > > > > > > From that we can derive a object for QMP Clients, giving slightly higher > > level APIs to send commands, receive replies & events, avoiding the need > > to touch the "Message" object directly: > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/client.go > > > > And derive a similar object for QMP severs, giving APIs to dispatch > > commands, > > and send replies, events & errors: > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/server.go > > > > So far this is all static written code common to any QAPI schema impl. > > The APIs can be used with any schema specific structs, however, the > > latter might be defined. > > > > > > Being low level, none of these APIs are strongly typed, which is not nice > > as an application API in Go, so need a way to introduce better type safety. > > > > > > In a real application I don't think developers should really be touching > > the structs directly for commands/responses/events. Instead I believe > > they need to be given formal APIs: > > > > Thus I have (what would be) auto-generated interfaces for covering all > > the commands and events: > > > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_commands.go > > > > https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_events.go > > One thing that bothers me a little is using event's fields > directly instead of event type in the interface itself: > > type Events interface { > StopEvent(time.Time) error > ContEvent(time.Time) error > BalloonChangeEvent(int64, time.Time) error > } > > In the gen_server_events.go: > > func (cmds *serverEvents) BalloonChangeEvent(actual int64, when > time.Time) error { > ev := &BalloonChangeEvent{ > Actual: actual, > } > return cmds.QMP.SendEvent("BALLOON_CHANGE", ev, when) > } > > I'm actually in favor of moving the Type itself to the method: > > type Events interface { > StopEvent(time.Time) error > ContEvent(time.Time) error > BalloonChangeEvent(BalloonChangeEvent, time.Time) error > } > > func (cmds *serverEvents) BalloonChangeEvent(ev BalloonChangeEvent, when > time.Time) error { > return cmds.QMP.SendEvent("BALLOON_CHANGE", &ev, when) > }
I choose to expand all the types inline in events & commands because I felt like this gives a more convenient API design for applications, without the extra indirectino of packing and unpacking parameters . > For the caller it might be an extra step but I see a few > benefits: > > 1. Documentation: I'm generating the QAPI documentation of > the events in its types. IDE's can jump-to-definition and > see the documentation there. Obviously we could move the > documentation here or duplicate it but the doc in the Type > looks the right place imho. Yep, it would complicate docs to have to unpack it against the method. I was coming at this from the POV of an application developer though, intentionally ignoring what is the most convenient for the code generator. The latter is fixed cost while writing the generator, while the API design is an ongoing cost time, so makes more sense to optimize for the latter. > 2. Changes in the Event fields over time would not impact the > Breaking the Methods API. While some fields my be > added/removed/changed, I think this would help break less > the application when bumping the go module version. Yep, hiding everything behind a struct can reduce breakage. The tricky (impossible) question is how beneficial it is in the real world usage ? As guidance, taking protobuf as a commonly used package though, if I look at Kubevirt's protobuf generated APIs: https://github.com/kubevirt/kubevirt/blob/main/pkg/handler-launcher-com/cmd/v1/cmd.pb.go#L1237 I can see they're using structs for parameters and return values. So that widely used prior art, suggests we go the way you outline and use structs. > I'm not sure about empty types like StopEvent. I'm generating it > with its doc but as it goes, it is bound to not be used, being > generated just as source of documentation. Still, if it so > happens that it does change in the future, we would need to > extend the Method with its field or its type. > > // Emitted when the virtual machine is stopped > // > // Since: 0.12 > // > // .. qmp-example:: <- { "event": "STOP", "timestamp": { > // "seconds": 1267041730, "microseconds": 281295 } } > type StopEvent struct{} Yes, if the goal of using structs is to reduce breakage when fields are added/removed, then IMHO the logical conclusion is that empty structs must be generated. > That also applies for commands.. > > // Resume guest VM execution. > // > // Since: 0.14 > // > // .. note:: This command will succeed if the guest is currently > // running. It will also succeed if the guest is in the "inmigrate" > // state; in this case, the effect of the command is to make sure > // the guest starts once migration finishes, removing the effect of > // the "-S" command line option if it was passed. If the VM was > // previously suspended, and not been reset or woken, this command > // will transition back to the "suspended" state. (Since 9.0) .. > // qmp-example:: -> { "execute": "cont" } <- { "return": {} } > type ContCommand struct {} > > > I honestly did the requested changes a little while ago but I was > thinking about this and tinkering a little with the demo. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
