On Sat, Apr 02, 2022 at 12:40:56AM +0200, Victor Toso wrote: > Thanks for taking a look, let me know if you have questions, ideas > or suggestions.
Full disclosure: I have only given the actual implementation a very cursory look so far, and I've focused on the generated Go API instead. Overall things look pretty good. One concern that I have is about naming struct members: things like SpiceInfo.MouseMode and most others are translated from the QAPI schema exactly the way you'd expect them, but for example ChardevCommon.Logappend doesn't look quite right. Of course there's no way to programmatically figure out what to capitalize, but maybe there's room for adding this kind of information in the form of additional annotations or something like that? Same for the various structs or members that have unexpectedly-capitalized "Tls" or "Vnc" in them. To be clear, I don't think the above is a blocker - just something to be aware of, and think about. My biggest concern is about the interface offered for commands. Based on the example you have in the README and how commands are defined, invoking (a simplified version of) the trace-event-get-state command would look like cmd := Command{ Name: "trace-event-get-state", Arg: TraceEventGetStateCommand{ Name: "qemu_memalign", }, } qmp_input, _ := json.Marshal(&cmd) // qmp_input now contains // {"execute":"trace-event-get-state","arguments":{"name":"qemu_memalign"}} // do something with it qmp_output := ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`) ret := cmd.GetReturnType() _ = json.Unmarshal(qmp_output, &ret) // ret is a CommandResult instance whose Value member can be cast // to a TraceEventInfo struct First of all, from an application's point of view there are way too many steps involved: performing this operation should really be as simple as ret, _ := qmp.TraceEventGetState("qemu_memalign") // ret is a TraceEventInfo instance That's the end state we should be working towards. Of course that assumes that the "qmp" object knows where the QMP socket is, knows how to talk the QMP protocol, transparently deals with serializing and deserializing data... Plus, in some case you might want to deal with the wire transfer yourself in an application-specific manner. So it makes sense to have the basic building blocks available and then build the more ergonomic SDK on top of that - with only the first part being in scope for this series. Even with that in mind, the current interface is IMO problematic because of its almost complete lack of type safety. Both Command.Arg and CommandResult.Value are of type Any and CommandBase.Name, which is used to drive the JSON unmarshal logic as well as ending up on the wire when executing a command, is just a plain string. I think the low-level interface should look more like cmd := TraceEventGetStateCommand{ Name: "qemu_memalign", } qmp_input, _ := json.Marshal(&cmd) // qmp_input looks the same as before qmp_output := ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`) ret := TraceEventInfo{} _ = json.Unmarshal(qmp_output, &ret) // ret is a TraceEventInfo instance The advantages over the current implementation is that the compiler will prevent you from doing something silly like passing the wrong set of arguments to a commmand, and that the application has to explicitly spell out what kind of object it expects to get as output. I'm attaching an incomplete implementation that I used for playing around. It's obviously too simplistic, but hopefully it will help illustrate my point. Dealing with errors and commands that don't have a return value might require us to have generic CommandResult wrapper after all, but we should really try as hard as we can to stick to type safe interfaces. -- Andrea Bolognani / Red Hat / Virtualization
package main import ( "encoding/json" "fmt" ) type TraceEventGetStateCommand struct { Name string `json:"name"` } func (self *TraceEventGetStateCommand) MarshalJSON() ([]byte, error) { type Arguments TraceEventGetStateCommand return json.Marshal(&struct { Execute string `json:"execute"` Arguments *Arguments `json:"arguments"` }{ Execute: "trace-event-get-state", Arguments: (*Arguments)(self), }) } type TraceEventInfo struct { Name string `json:"name"` State string `json:"state"` } func (self *TraceEventInfo) UnmarshalJSON(data []byte) error { type Return TraceEventInfo ret := struct { Return Return `json:"return"` }{} err := json.Unmarshal(data, &ret) if err != nil { return err } self.Name = ret.Return.Name self.State = ret.Return.State return nil } func main() { var err error var qmp_input []byte var qmp_output []byte cmd := TraceEventGetStateCommand{ Name: "qemu_memalign", } if qmp_input, err = json.Marshal(&cmd); err != nil { panic(err) } fmt.Printf(" cmd: %v\n", cmd) fmt.Printf("-> qmp_input: %v\n", string(qmp_input)) qmp_output = ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`) ret := TraceEventInfo{} if err = json.Unmarshal(qmp_output, &ret); err != nil { panic(err) } fmt.Printf("<- qmp_output: %v\n", string(qmp_output)) fmt.Printf(" ret: %v\n", ret) }