Re: [PATCH v3 0/8] qapi-go: add generator for Golang interfaces

[Victor Toso]

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

signature.asc
Description: PGP signature