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)
}
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.
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.
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{}
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.
Cheers,
Victor
> these interfaces are applicable to both clients and servers, just needing
> different private implementations.
>
> On the client side the implementations of these interfaces:
>
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_client_sync.go
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_client_async.go
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_client_events.go
>
> And on the server side the implementations of these interfaces
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_server.go
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/pkg/qapi/gen_server_events.go
>
>
> Finally we can consider what an implementation of a client looks like
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/cmd/sync-client/main.go
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/cmd/async-client/main.go
>
> And what an implementation of a server looks like
>
> https://gitlab.com/berrange/qapi-go-demo/-/blob/main/cmd/server/main.go
>
> The key observation at the end is that the client/server impls are all
> using strongly typed APIs and don't have to concern themselves with JSON
> at all.
>
> The two demo clients should work with an real QEMU or the demo server.
> The demo server won't work with qmp-shell, because I blindly assumed
> each JSON message had a terminating newlinue for ease of impl and this
> isn't compatible with qmp-shell. Solvable but I couldn't be bothered
> for this demoware.
>
>
> So what does this all mean wrt your series ? Not that much, probably
> just a little code deletion and some small tweaks.
>
>
> First, it illustrates that the approach taken for the Command / Event
> interfaces & the corresponding UnmarshallJSON/MarshallJSON methods is
> redundant code. Aside from that, I think everything else in your series
> around generating the basic types is essentially good.
>
> Second, it shows a way to build on top of your series, to define a higher
> level API to make interacting with QMP much easier for app developers,
> eliminating all exposure of JSON & marshalling, in favour of formal APIs.
> This doesn't have to be done now, it can be a phase two, as long as we
> have an approximate idea of what the phase two API will look like, so
> we can validate the phase one design against these future plans.
>
>
> NB What I've not especially considered in any of this is the impact of
> differing QEMU versions & their schema changes. The easy way out is to
> just re-run the generator for each version, putting them in a separate
> Go package. So you can do 'import' of "qapi/qemu/abi920" for 9.2.0
> schema APIs, or "qapi/qemu/910" for 9.1.0 schema APIs. The app dev would
> have to choose which version (or versions) they consume & implement
> against. Splitting versions across the whole package, avoids having to
> consider versioning within parameters of a single command/event.
>
> 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 :|
>
signature.asc
Description: PGP signature
