docs/devel/index-build.rst | 1 + docs/devel/qapi-golang-code-gen.rst | 548 +++++++++ scripts/qapi/golang.py | 1645 +++++++++++++++++++++++++++ scripts/qapi/main.py | 3 + 4 files changed, 2197 insertions(+) create mode 100644 docs/devel/qapi-golang-code-gen.rst create mode 100644 scripts/qapi/golang.py
This patch series intent is to introduce a generator that produces a Go
module for Go applications to interact over QMP with QEMU.
The initial Goal is to have a Go module that works as intended and can
be improved upon. I'd consider initial releases to be alpha while we
work with utilities tools and libraries on top of this.
The generated code should reside in a separated Git repository, similar
to python-qemu-qmp.
Applications should be able to consume this under qemu.org
namespace (e.g: import "qemu.org/go/qemu"), see Daniel's suggestion:
https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg07024.html
This is the third iteration:
v2: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04785.html
I've pushed this series in my gitlab fork:
https://gitlab.com/victortoso/qapi-go/
The fork contains some tests, including tests that were generated from
QAPI's own examples from another generator created for testing, if you
are interested in it:
https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html
I've generated the qapi-go module over each commit of this series, see:
https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch
I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0:
https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags
--
Sorry that its been awhile between v2 and v3, I had to prioritize other
things. I hope to get this back on track in 2025.
Cheers,
Victor
* Changes:
On generated go:
- the output should be formatted as gofmt/goimports tools (Daniel)
- Included QAPI's documentation too (Daniel), see:
https://lists.gnu.org/archive/html/qemu-devel/2024-11/msg01621.html
- Commands and Events should Marshal directly (Andrea)
On python script:
- rebased: now uses QAPISchemaBranches, QAPISchemaAlternatives
- use textwrap as much as possible (Andrea)
- lots of changes to make the output like gofmt does
Victor Toso (8):
qapi: golang: Generate enum type
qapi: golang: Generate alternate types
qapi: golang: Generate struct types
qapi: golang: structs: Address nullable members
qapi: golang: Generate union type
qapi: golang: Generate event type
qapi: golang: Generate command type
docs: add notes on Golang code generator
docs/devel/index-build.rst | 1 +
docs/devel/qapi-golang-code-gen.rst | 548 +++++++++
scripts/qapi/golang.py | 1645 +++++++++++++++++++++++++++
scripts/qapi/main.py | 3 +
4 files changed, 2197 insertions(+)
create mode 100644 docs/devel/qapi-golang-code-gen.rst
create mode 100644 scripts/qapi/golang.py
--
2.47.1
Victor Toso <victortoso@redhat.com> writes: > This patch series intent is to introduce a generator that produces a Go > module for Go applications to interact over QMP with QEMU. > > The initial Goal is to have a Go module that works as intended and can > be improved upon. I'd consider initial releases to be alpha while we > work with utilities tools and libraries on top of this. > > The generated code should reside in a separated Git repository, similar > to python-qemu-qmp. > > Applications should be able to consume this under qemu.org > namespace (e.g: import "qemu.org/go/qemu"), see Daniel's suggestion: > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg07024.html > > This is the third iteration: > v2: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04785.html > > I've pushed this series in my gitlab fork: > https://gitlab.com/victortoso/qapi-go/ > > The fork contains some tests, including tests that were generated from > QAPI's own examples from another generator created for testing, if you > are interested in it: > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html > > I've generated the qapi-go module over each commit of this series, see: > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch > > I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0: > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags > > -- > > Sorry that its been awhile between v2 and v3, I had to prioritize other > things. I hope to get this back on track in 2025. > > Cheers, > Victor > > * Changes: > > On generated go: > - the output should be formatted as gofmt/goimports tools (Daniel) > > - Included QAPI's documentation too (Daniel), see: > https://lists.gnu.org/archive/html/qemu-devel/2024-11/msg01621.html > > - Commands and Events should Marshal directly (Andrea) > > On python script: > - rebased: now uses QAPISchemaBranches, QAPISchemaAlternatives > > - use textwrap as much as possible (Andrea) > > - lots of changes to make the output like gofmt does > > Victor Toso (8): > qapi: golang: Generate enum type > qapi: golang: Generate alternate types > qapi: golang: Generate struct types > qapi: golang: structs: Address nullable members > qapi: golang: Generate union type > qapi: golang: Generate event type > qapi: golang: Generate command type > docs: add notes on Golang code generator > > docs/devel/index-build.rst | 1 + > docs/devel/qapi-golang-code-gen.rst | 548 +++++++++ > scripts/qapi/golang.py | 1645 +++++++++++++++++++++++++++ > scripts/qapi/main.py | 3 + > 4 files changed, 2197 insertions(+) > create mode 100644 docs/devel/qapi-golang-code-gen.rst > create mode 100644 scripts/qapi/golang.py This is series adds a backend that slots in cleanly, i.e. without any changes to the core. That makes it as low-risk to merge as it gets. I'd like an Acked-by for the generated Go from someone familiar the kind of software that could use it. The Go backend is a single golang.py, which generates: * Types: alternates.go enums.go structs.go unions.go * Commands: commands.go * Events: events.go It doesn't generate visitors or introspection code. Correct? The existing C backend generates code for * Types (types.py): qapi-builtin-types.[ch] qapi-types.[ch] qapi-types-*.[ch] * Visitors (visit.py): qapi-builtin-visit.h qapi-visit.[ch] qapi-visit-*.[ch] * Commands (commands.py): qapi-init-commands.h qapi-commands.[ch] qapi-commands-*.[ch] * Events (events.py): qapi-emit-events.h qapi-events.[ch] qapi-events-*.[ch] * Introspection (introspect.py): qapi-introspect.h The -* files are all one pair of files per module (the things pulled in with include directives), if any. We do this to avoid "touch the QAPI schema, recompile the world." The generated Go is monolithic. No "recompile the world" problem with Go? golang.py is somewhat big. Whether splitting it up along the lines of the C backend would improve things I can't say. No need to worry about that now.
On Mon, Jan 13, 2025 at 01:52:25PM +0100, Markus Armbruster wrote: > Victor Toso <victortoso@redhat.com> writes: > > > This patch series intent is to introduce a generator that produces a Go > > module for Go applications to interact over QMP with QEMU. > > > > The initial Goal is to have a Go module that works as intended and can > > be improved upon. I'd consider initial releases to be alpha while we > > work with utilities tools and libraries on top of this. > > > > The generated code should reside in a separated Git repository, similar > > to python-qemu-qmp. > > > > Applications should be able to consume this under qemu.org > > namespace (e.g: import "qemu.org/go/qemu"), see Daniel's suggestion: > > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg07024.html > > > > This is the third iteration: > > v2: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04785.html > > > > I've pushed this series in my gitlab fork: > > https://gitlab.com/victortoso/qapi-go/ > > > > The fork contains some tests, including tests that were generated from > > QAPI's own examples from another generator created for testing, if you > > are interested in it: > > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html > > > > I've generated the qapi-go module over each commit of this series, see: > > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch > > > > I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0: > > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags > > > > -- > > > > Sorry that its been awhile between v2 and v3, I had to prioritize other > > things. I hope to get this back on track in 2025. > > > > Cheers, > > Victor > > > > * Changes: > > > > On generated go: > > - the output should be formatted as gofmt/goimports tools (Daniel) > > > > - Included QAPI's documentation too (Daniel), see: > > https://lists.gnu.org/archive/html/qemu-devel/2024-11/msg01621.html > > > > - Commands and Events should Marshal directly (Andrea) > > > > On python script: > > - rebased: now uses QAPISchemaBranches, QAPISchemaAlternatives > > > > - use textwrap as much as possible (Andrea) > > > > - lots of changes to make the output like gofmt does > > > > Victor Toso (8): > > qapi: golang: Generate enum type > > qapi: golang: Generate alternate types > > qapi: golang: Generate struct types > > qapi: golang: structs: Address nullable members > > qapi: golang: Generate union type > > qapi: golang: Generate event type > > qapi: golang: Generate command type > > docs: add notes on Golang code generator > > > > docs/devel/index-build.rst | 1 + > > docs/devel/qapi-golang-code-gen.rst | 548 +++++++++ > > scripts/qapi/golang.py | 1645 +++++++++++++++++++++++++++ > > scripts/qapi/main.py | 3 + > > 4 files changed, 2197 insertions(+) > > create mode 100644 docs/devel/qapi-golang-code-gen.rst > > create mode 100644 scripts/qapi/golang.py > > This is series adds a backend that slots in cleanly, i.e. without any > changes to the core. That makes it as low-risk to merge as it gets. > > I'd like an Acked-by for the generated Go from someone familiar the kind > of software that could use it. In my other (huge) reply to this thread I tried to provide that analysis by imagining how I would want to consume a QEMU API in Go, creating an example application and trying to see how well it fits with this design. > The -* files are all one pair of files per module (the things pulled in > with include directives), if any. We do this to avoid "touch the QAPI > schema, recompile the world." > > The generated Go is monolithic. No "recompile the world" problem with > Go? IIUC, the intent is that we dn't generate the go code as part of the regular QEMU build. IOW, most of the time it would be generate once against a release tag. If someone was actively working on QAPI schema on git master, at the same time as developing a Go application, they'll have a bit more of a repeated compile penalty. I'm not convinced that's a big enough common case to worry about modularizing though. > golang.py is somewhat big. Whether splitting it up along the lines of > the C backend would improve things I can't say. No need to worry about > that now. With the monolothic go code, generated once per release tag, Go is going to be reusing cached previously compiled objects most of the time. I think that's likely to be good enough. 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 :|
Hi, On Mon, Jan 13, 2025 at 01:52:25PM +0100, Markus Armbruster wrote: > Victor Toso <victortoso@redhat.com> writes: > > > This patch series intent is to introduce a generator that produces a Go > > module for Go applications to interact over QMP with QEMU. > > > > The initial Goal is to have a Go module that works as intended and can > > be improved upon. I'd consider initial releases to be alpha while we > > work with utilities tools and libraries on top of this. > > > > The generated code should reside in a separated Git repository, similar > > to python-qemu-qmp. > > > > Applications should be able to consume this under qemu.org > > namespace (e.g: import "qemu.org/go/qemu"), see Daniel's suggestion: > > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg07024.html > > > > This is the third iteration: > > v2: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04785.html > > > > I've pushed this series in my gitlab fork: > > https://gitlab.com/victortoso/qapi-go/ > > > > The fork contains some tests, including tests that were generated from > > QAPI's own examples from another generator created for testing, if you > > are interested in it: > > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html > > > > I've generated the qapi-go module over each commit of this series, see: > > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch > > > > I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0: > > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags > > > > -- > > > > Sorry that its been awhile between v2 and v3, I had to prioritize other > > things. I hope to get this back on track in 2025. > > > > Cheers, > > Victor > > > > * Changes: > > > > On generated go: > > - the output should be formatted as gofmt/goimports tools (Daniel) > > > > - Included QAPI's documentation too (Daniel), see: > > https://lists.gnu.org/archive/html/qemu-devel/2024-11/msg01621.html > > > > - Commands and Events should Marshal directly (Andrea) > > > > On python script: > > - rebased: now uses QAPISchemaBranches, QAPISchemaAlternatives > > > > - use textwrap as much as possible (Andrea) > > > > - lots of changes to make the output like gofmt does > > > > Victor Toso (8): > > qapi: golang: Generate enum type > > qapi: golang: Generate alternate types > > qapi: golang: Generate struct types > > qapi: golang: structs: Address nullable members > > qapi: golang: Generate union type > > qapi: golang: Generate event type > > qapi: golang: Generate command type > > docs: add notes on Golang code generator > > > > docs/devel/index-build.rst | 1 + > > docs/devel/qapi-golang-code-gen.rst | 548 +++++++++ > > scripts/qapi/golang.py | 1645 +++++++++++++++++++++++++++ > > scripts/qapi/main.py | 3 + > > 4 files changed, 2197 insertions(+) > > create mode 100644 docs/devel/qapi-golang-code-gen.rst > > create mode 100644 scripts/qapi/golang.py > > This is series adds a backend that slots in cleanly, i.e. without any > changes to the core. That makes it as low-risk to merge as it gets. > > I'd like an Acked-by for the generated Go from someone familiar the kind > of software that could use it. > > The Go backend is a single golang.py, which generates: > > * Types: > alternates.go enums.go structs.go unions.go > > * Commands: > commands.go > > * Events: > events.go > > It doesn't generate visitors or introspection code. > > Correct? Correct. I've actually thought about following what we did with libvirt-go-module which appends _generated to the files that are generated, meaning that we would also have files that are without _generated in the name, with custoam/manual code related to that file. Either way, that does not change the Go module (does not break API/ABI) so we can customize it if needed at a later time. > The existing C backend generates code for > > * Types (types.py): > qapi-builtin-types.[ch] > qapi-types.[ch] qapi-types-*.[ch] > > * Visitors (visit.py): > > qapi-builtin-visit.h > qapi-visit.[ch] qapi-visit-*.[ch] > > * Commands (commands.py): > > qapi-init-commands.h > qapi-commands.[ch] qapi-commands-*.[ch] > > * Events (events.py): > > qapi-emit-events.h > qapi-events.[ch] qapi-events-*.[ch] > > * Introspection (introspect.py): > > qapi-introspect.h > > The -* files are all one pair of files per module (the things pulled in > with include directives), if any. We do this to avoid "touch the QAPI > schema, recompile the world." > > The generated Go is monolithic. No "recompile the world" problem with > Go? From my experience, Go compiler only builds when the .go file changes. If the QAPI changes and the generator outputs different .go files, it'll recompile at the project using it. > golang.py is somewhat big. Whether splitting it up along the lines of > the C backend would improve things I can't say. No need to worry about > that now. Yes, I'm not super experienced with python. I'm all ears to any suggestions on how to organize the source better. I'm happy to do it as a follow-up. Cheers, Victor
On Fri, Jan 10, 2025 at 11:49:38AM +0100, Victor Toso wrote:
> I've pushed this series in my gitlab fork:
> https://gitlab.com/victortoso/qapi-go/
>
> The fork contains some tests, including tests that were generated from
> QAPI's own examples from another generator created for testing, if you
> are interested in it:
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html
>
> I've generated the qapi-go module over each commit of this series, see:
> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch
>
> I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0:
> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags
Apologies in advance for what will be a long mail.....
I can't recall all the discussions we had on previous versions now, but
I've been having a look at the generated code again, and have thoughts
around the design of the Event / Command types.
I've been wondering how to actually use the generated code to implement
a client and a server API. We previously said such APIs are out of scope
for this series, but at the same time I think they are pretty important
to consider as a way to evaluate the design approach.
Sending messages is easy as you always know what you're sending. The
big design issue is around receiving data off the wire, and the
associated unmarshalling. QMP has several different message types
(command, return, event, error, QMP greeting). When we've read a JSON
JSON doc off the wire, we (usually) have no idea what message type we
are going to unmarshal. Even if we know the message type, we then
might not know the sub-type.
IOW we have a major chicken & egg problem with unmarshalling.
Taking BalloonChangeEvent:
type BalloonChangeEvent struct {
MessageTimestamp Timestamp `json:"-"`
// the logical size of the VM in bytes Formula used:
// logical_vm_size = vm_ram_size - balloon_size
Actual int64 `json:"actual"`
}
which has an UnmarshallJSON method that consumes this whole doc:
{ "event": "BALLOON_CHANGE",
"data": { "actual": 944766976 },
"timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
Before we can instantiate a BalloonChangeEvent struct and call its
UnmarshallJSON method, we have to already know we've received an
event and also know the event type is for BalloonChangeEvent.
This means we must have already partially unmarshalled it before
we can unmarshall into BalloonChangeEvent.
IMHO the idea of having UnmarshallJSON methods on the "Event" interface
(and "Command" interface) that consumes a complete message is thus
flawed.
Instead, I believe we need to have a two phase approach to unmarshalling.
For the 1st phase we need a struct which can unmarshall *any* type of
QMP message, but only 1 level deep. Essentially something like this:
type Message struct {
QMP *json.RawMessage `json:"QMP,omitempty"`
Execute string `json:"execute,omitempty"`
ExecOOB string `json:"exec-oob,omitempty"`
Event string `json:"event,omitempty"`
Error *json.RawMessage `json:"error,omitempty"`
Return *json.RawMessage `json:"return,omitempty"`
ID string `json:"id,omitempty"`
Timestamp *Timestamp `json:"timestamp,omitempty"`
Data *json.RawMessage `json:"data,omitempty"`
Arguments *json.RawMessage `json:"arguments,omitempty"`
}
Once that Message is unmarshalled, we can examine the 'QMP', 'Execute'
'ExecOOB', 'Event', 'Error' and 'Return' fields, to understand what type
of message we've received.
Then it is possible to identify the right Event or Command or Return
class to use to perform the second level of unmarshalling using the
json.RawMessage data the 1st level of unmarshalling captured.
IOW, if 'Event' is 'BALLOON_CHANGE_EVENT', we know we can use a
BalloonChangeEvent struct to Unmarshall. This struct, however, would
look different to what you've proposed, it would only need to contain
the 'data' fields for the event
type BalloonChangeEvent struct {
// the logical size of the VM in bytes Formula used:
// logical_vm_size = vm_ram_size - balloon_size
Actual int64 `json:"actual"`
}
This would also mean there is no need to provide any UnmarshallJSON
or MarshallJSON methods at all on the BalloonChangeEvent, as the json
marshall code can work exclusively from the field annotations.
The same thoughts above apply to the command / return structs. I don't
think any of them need to have UnmarshallJSON/MarshallJSON methods.
<<<< 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
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 :|
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 :|
>
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 :|
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
Hi,
On Thu, Jan 16, 2025 at 09:59:52PM +0000, Daniel P. Berrangé wrote:
> On Fri, Jan 10, 2025 at 11:49:38AM +0100, Victor Toso wrote:
> > I've pushed this series in my gitlab fork:
> > https://gitlab.com/victortoso/qapi-go/
> >
> > The fork contains some tests, including tests that were generated from
> > QAPI's own examples from another generator created for testing, if you
> > are interested in it:
> > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html
> >
> > I've generated the qapi-go module over each commit of this series, see:
> > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch
> >
> > I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0:
> > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags
>
> Apologies in advance for what will be a long mail.....
>
> I can't recall all the discussions we had on previous versions now, but
> I've been having a look at the generated code again, and have thoughts
> around the design of the Event / Command types.
>
>
> I've been wondering how to actually use the generated code to implement
> a client and a server API. We previously said such APIs are out of scope
> for this series, but at the same time I think they are pretty important
> to consider as a way to evaluate the design approach.
>
> Sending messages is easy as you always know what you're sending. The
> big design issue is around receiving data off the wire, and the
> associated unmarshalling. QMP has several different message types
> (command, return, event, error, QMP greeting). When we've read a JSON
> JSON doc off the wire, we (usually) have no idea what message type we
> are going to unmarshal. Even if we know the message type, we then
> might not know the sub-type.
>
> IOW we have a major chicken & egg problem with unmarshalling.
>
> Taking BalloonChangeEvent:
>
> type BalloonChangeEvent struct {
> MessageTimestamp Timestamp `json:"-"`
> // the logical size of the VM in bytes Formula used:
> // logical_vm_size = vm_ram_size - balloon_size
> Actual int64 `json:"actual"`
> }
>
> which has an UnmarshallJSON method that consumes this whole doc:
>
> { "event": "BALLOON_CHANGE",
> "data": { "actual": 944766976 },
> "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>
> Before we can instantiate a BalloonChangeEvent struct and call its
> UnmarshallJSON method, we have to already know we've received an
> event and also know the event type is for BalloonChangeEvent.
> This means we must have already partially unmarshalled it before
> we can unmarshall into BalloonChangeEvent.
>
> IMHO the idea of having UnmarshallJSON methods on the "Event" interface
> (and "Command" interface) that consumes a complete message is thus
> flawed.
>
>
> Instead, I believe we need to have a two phase approach to unmarshalling.
>
> For the 1st phase we need a struct which can unmarshall *any* type of
> QMP message, but only 1 level deep. Essentially something like this:
>
> type Message struct {
> QMP *json.RawMessage `json:"QMP,omitempty"`
> Execute string `json:"execute,omitempty"`
> ExecOOB string `json:"exec-oob,omitempty"`
> Event string `json:"event,omitempty"`
> Error *json.RawMessage `json:"error,omitempty"`
> Return *json.RawMessage `json:"return,omitempty"`
> ID string `json:"id,omitempty"`
> Timestamp *Timestamp `json:"timestamp,omitempty"`
> Data *json.RawMessage `json:"data,omitempty"`
> Arguments *json.RawMessage `json:"arguments,omitempty"`
> }
/* Note to self, should have probably tested 'exec-oob' */
> Once that Message is unmarshalled, we can examine the 'QMP', 'Execute'
> 'ExecOOB', 'Event', 'Error' and 'Return' fields, to understand what type
> of message we've received.
>
> Then it is possible to identify the right Event or Command or Return
> class to use to perform the second level of unmarshalling using the
> json.RawMessage data the 1st level of unmarshalling captured.
>
>
> IOW, if 'Event' is 'BALLOON_CHANGE_EVENT', we know we can use a
> BalloonChangeEvent struct to Unmarshall. This struct, however, would
> look different to what you've proposed, it would only need to contain
> the 'data' fields for the event
>
> type BalloonChangeEvent struct {
> // the logical size of the VM in bytes Formula used:
> // logical_vm_size = vm_ram_size - balloon_size
> Actual int64 `json:"actual"`
> }
>
> This would also mean there is no need to provide any UnmarshallJSON
> or MarshallJSON methods at all on the BalloonChangeEvent, as the json
> marshall code can work exclusively from the field annotations.
>
> The same thoughts above apply to the command / return structs. I don't
> think any of them need to have UnmarshallJSON/MarshallJSON methods.
>
>
>
> <<<< 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
>
> 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.
Alright. The addition of Marshallers was done in the last
interaction. Before, I had the following for command and similar
to event:
func MarshalCommand(c Command) ([]byte, error)
func UnmarshalCommand(data []byte) (Command, error)
Each of this series iterations had a more or less a different
idea of how the consumer of this generated code would use it, but
without a proper PoC on top, like you've done here and thanks for
that.
I'm happy with the suggestions and I'll incorporate them.
> 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.
Just want to put emphasis that the module that we will maintain
would still be considered alpha/unstable until we get the library
in a state we agree is good.
> 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.
Yeah, plus what Markus said. Choosing qapi/qemu/900 should stay
working as long as QMP would guarantee (end of grace period of a
breaking change).
I'd consider sane Applications to bump the go module when they
bump the minimum required QEMU version for their supported use
case, to be able to handle new events and changes.
> 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.
Thanks again,
Victor
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Jan 10, 2025 at 11:49:38AM +0100, Victor Toso wrote: >> I've pushed this series in my gitlab fork: >> https://gitlab.com/victortoso/qapi-go/ >> >> The fork contains some tests, including tests that were generated from >> QAPI's own examples from another generator created for testing, if you >> are interested in it: >> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04946.html >> >> I've generated the qapi-go module over each commit of this series, see: >> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-patch >> >> I've also generated the qapi-go module over QEMU tags: v9.1.0, v9.2.0: >> https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v3-by-tags > > Apologies in advance for what will be a long mail..... I decline the opportunity for the pot to call the kettle black ;) [...] > 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. I endorse this approach. First, a practical point. We want to generate bindings from the schema. The generator we have works on the current version of the schema. To generate something spanning versions, we would have to make it work on several versions, wouldn't we? I don't think we can afford such ambition. Next, some supporting thoughts on interfaces and why bindings spanning versions feels like way too much trouble to me. QMP was designed for compatible schema evolution: we can evolve the schema without breaking clients of the wire interface as long as they stick to certain rules, such as "ignore unknown members in client input". Occasionally, the schema needs to change in ways that can affect such clients. We promise to give clients notice and time to adjust: we formally deprecate the old interface, but keep it working for a grace period, namely the release it was deprecated and one further release. So, if your QMP client targets a specific version of the wire interface, and avoids deprecated stuff, it'll be fine for at least two more releases. After that, you better upgrade the client. This is a moderately flexible coupling between QMP client and server. When you stretch it beyond the limit, things for the most part continue to work. Only the things we changed incompatibly can break. If you need more flexible coupling, you can use QMP introspection to target a *range* of wire interfaces. The price is additional complexity in the client. Downstreams complicate this story a bit, but not in interesting ways, I believe. The existing C bindings are for the current schema version only. That's fine, because the only customer (QEMU) only cares for the current version. It would also be fine for external customers as long as they only need the moderately flexible coupling described above. Bindings spanning schema versions are significantly harder. Do our rules for compatible schema evolution transfer from the JSON-based wire interface to the bindings? Not necessarily. May require acrobatics that make the bindings ugly. Example: adding an argument to an event is compatible evolution. Adding it to the C function to send the event is not compatible: it breaks the build, and all callers need to be adjusted to fix it. Yes, this may not be an issue with certain other languages, but my point stands: not necessarily. Example: adding a member to an object type is compatible evolution. Adding it to the type in the ABI is not compatible: mixing ABI before and after won't link if you're lucky, and run amok at run time if you're not.
© 2016 - 2025 Red Hat, Inc.