Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface

[Andrea Bolognani]

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

command.go
Description: Text document