Hi, On Tue, Feb 11, 2025 at 11:10:37AM +0000, Daniel P. Berrangé wrote: > 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
Sure, just a choice to be made.
> > 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.
Right. I don't mind doing the work to inline it, but I'm not sure
it is best. Inline commands/events that don't have fields are
straight forward and look nice but what when they have several
fields instead?
Wrt to mandatory and optional fields, I think it is
straightforward when looking at the struct definition that might
be confusing to the methods args.
As example, block-stream command has 12 fields, only 1 mandatory.
// .. qmp-example:: -> { "execute": "block-stream",
// "arguments": { "device": "virtio0", "base":
// "/tmp/master.qcow2" } } <- { "return": {} }
type BlockStreamCommand struct {
// identifier for the newly-created block job. If omitted, the
// device name will be used. (Since 2.7)
JobId *string `json:"job-id,omitempty"`
// the device or node name of the top image
Device string `json:"device"`
// the common backing file name. It cannot be set if @base-node or
// @bottom is also set.
Base *string `json:"base,omitempty"`
// the node name of the backing file. It cannot be set if @base or
// @bottom is also set. (Since 2.8)
BaseNode *string `json:"base-node,omitempty"`
// The backing file string to write into the top image. This
// filename is not validated. If a pathname string is such that it
// cannot be resolved by QEMU, that means that subsequent QMP or
// HMP commands must use node-names for the image in question, as
// filename lookup methods will fail. If not specified, QEMU will
// automatically determine the backing file string to use, or
// error out if there is no obvious choice. Care should be taken
// when specifying the string, to specify a valid filename or
// protocol. (Since 2.1)
BackingFile *string `json:"backing-file,omitempty"`
// If true, replace any protocol mentioned in the 'backing file
// format' with 'raw', rather than storing the protocol name as
// the backing format. Can be used even when no image header will
// be updated (default false; since 9.0).
BackingMaskProtocol *bool `json:"backing-mask-protocol,omitempty"`
// the last node in the chain that should be streamed into top. It
// cannot be set if @base or @base-node is also set. It cannot be
// filter node. (Since 6.0)
Bottom *string `json:"bottom,omitempty"`
// the maximum speed, in bytes per second
Speed *int64 `json:"speed,omitempty"`
// the action to take on an error (default report). 'stop' and
// 'enospc' can only be used if the block device supports io-
// status (see BlockInfo). (Since 1.3)
OnError *BlockdevOnError `json:"on-error,omitempty"`
// the node name that should be assigned to the filter driver that
// the stream job inserts into the graph above @device. If this
// option is not given, a node name is autogenerated. (Since: 6.0)
FilterNodeName *string `json:"filter-node-name,omitempty"`
// When false, this job will wait in a PENDING state after it has
// finished its work, waiting for @block-job-finalize before
// making any block graph changes. When true, this job will
// automatically perform its abort or commit actions. Defaults to
// true. (Since 3.1)
AutoFinalize *bool `json:"auto-finalize,omitempty"`
// When false, this job will wait in a CONCLUDED state after it
// has completely ceased all work, and awaits @block-job-dismiss.
// When true, this job will automatically disappear from the query
// list without user intervention. Defaults to true. (Since 3.1)
AutoDismiss *bool `json:"auto-dismiss,omitempty"`
}
Might be easier in that case to
cmds.BlockStream(BlockStreamCommand{device:"virtio"})
instead of defining all optional args as nil.
> > 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 ?
To me it was just an extra benefit, not a vital one. We are bound
to break when user bumps it. In the real world, as far as I
recall from discussing this with Markus (feel free to correct me)
i. Removing fields is somewhat rare. I remember he put out
an example but I could not find it.
ii. Adding fields is not so rare although they are usually
optional.
With inlining approach we would break application in both cases
100% of the time. It'd be less so by using a type.
> 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.
Oh, but if I'm not mistaken, if you do change the type you are
required to bump the version meaning that you'll need to
generated a new set of types with the new version. That enforces
client/side to implement the new type if they want to talk over
new version (enforced by capabilites negotiation). Not a great
dev experience but it surely works and performance is good too...
but I digress.
> > 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.
imho, just a extra benefit. Breaking is somewhat fine if it
happens only when we bump the version and is not something that
happens so often that it becomes a bother.
> > 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
Thanks for the quick replies,
Victor
signature.asc
Description: PGP signature
