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

[Andrea Bolognani]

On Mon, May 09, 2022 at 12:21:10PM +0200, Victor Toso wrote:
> On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> > 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:
>
> It can actually get worse. I've used a lot of nested struct to
> define a Base type for a given Type. In Go, If you try to
> initialize a Type that has a nested Struct, you'll need to use
> the nested struct Type as field name and this is too verbose.
>
> See https://github.com/golang/go/issues/29438 (merged with:
> https://github.com/golang/go/issues/12854)
>
> The main reason that I kept it is because it maps very well with
> the over-the-wire protocol.

Right, I had not realized how bad things really were :)

I've noticed the use of base types and while I didn't bring it up in
my initial message because the other concerns seemed of much higher
importance, I actually wondered whether we need to expose them to
users of the Go SDK.

I think we should flatten things. That's what happens with the C
generator already, for example in

  struct InetSocketAddress {
      /* Members inherited from InetSocketAddressBase: */
      char *host;
      char *port;
      /* Own members: */
      bool has_numeric;
      bool numeric;
      /* ... */
  };

This representation mirrors the wire protocol perfectly, so I see no
reason not to adopt it. Am I missing something?

> > 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.
>
> Right. Indeed, I thought a bit about what I want to fit into the
> code generator that will reside in QEMU and what we might want to
> develop on top of that.
>
> The goal for this series really is generating the data types that
> can be converted to/from QMP messages.

That's perfectly fine, and in fact I believe that splitting the whole
endeavor into three parts - QMP protocol implementation, QAPI types
serialization/deserialization, and a high-level SDK that gives easy
access to the previous two - is the best approach.

> >   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 think that, if we know all types that we can have at QAPI spec,
> the process of marshalling and unmarshalling should verify it.
> So, even if we don't change the expected interface as suggested,
> that work needs to be done. For some types, I've already did it,
> like for Unions and Alternate types.
>
> Example: 
> https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/unions.go#L28
>
> This union type can have 4 values for the Any interface type. The
> code generator documents it to help user's out.
>
>   | type SocketAddressLegacy struct {
>   |     // Base type for this struct
>   |     SocketAddressLegacyBase
>   |     // Value based on @type, possible types:
>   |     // * InetSocketAddressWrapper
>   |     // * UnixSocketAddressWrapper
>   |     // * VsockSocketAddressWrapper
>   |     // * StringWrapper
>   |     Value Any
>   | }
>
> On the Marshal function, I used Sprintf as a way to fetch Value's
> type. There are other alternatives but to the cost of adding
> other deps.
>
>   | func (s SocketAddressLegacy) MarshalJSON() ([]byte, error) {
>   |     base, err := json.Marshal(s.SocketAddressLegacyBase)
>   |     if err != nil {
>   |         return nil, err
>   |     }
>   |
>   |     typestr := fmt.Sprintf("%T", s.Value)
>   |     typestr =
>   |     typestr[strings.LastIndex(typestr, ".")+1:]
>
> ...
>
>   |     // "The branches need not cover all possible enum values"
>   |     // This means that on Marshal, we can safely ignore empty values
>   |     if typestr == "<nil>" {
>   |         return []byte(base), nil
>   |     }
>
> And then we have some Runtime checks to be sure to avoid the
> scenario mismatching Value's type.
>
>   |     // Runtime check for supported value types
>   |     if typestr != "StringWrapper" &&
>   |         typestr != "InetSocketAddressWrapper" &&
>   |         typestr != "UnixSocketAddressWrapper" &&
>   |         typestr != "VsockSocketAddressWrapper" {
>   |         return nil, errors.New(fmt.Sprintf("Type is not supported: %s", 
> typestr))
>   |    }
>   |    value, err := json.Marshal(s.Value)
>   |    if err != nil {
>   |        return nil, err
>   |    }
>
> With Alternate type, extra care was need on Unmarshal as we don't
> know the underlying type without looking at the message we
> received. That's the only reason of StrictDecode() helper
> function.
>
> I'm just pointing out with above examples that I agree with you
> with Type safety. It is hard to infer everything at compile-time
> so we need some Runtime checks. Having some nicer APIs will
> definitely help and improve developer experience too.

I agree that in some cases build time validation is simply not
possible, but this doesn't seem to be one of those cases.

For unmarshaling? Sure, especially if we keep in mind the forward
compatibility scenarios that Dan has raised elsewhere in the thread,
and that to be completely honest I haven't gotten around to fully
digesting yet.

But when it comes to marshaling we know ahead of time that the value
is going to be one of a handful of types, and we should get the
compiler to spot violations of this assumption for us. Using "Any"
prevents that.

If you look at libvirt-go-xml-module, alternates are handled with
things like

  type DomainInterfaceSource struct {
      Ethernet  *DomainInterfaceSourceEthernet `xml:"-"`
      Server    *DomainInterfaceSourceServer   `xml:"-"`
      // ...
  }

  type DomainInterfaceSourceEthernet struct {
      IP    []DomainInterfaceIP    `xml:"ip"`
      Route []DomainInterfaceRoute `xml:"route"`
  }

  type DomainInterfaceSourceServer struct {
      Address string                      `xml:"address,attr,omitempty"`
      Port    uint                        `xml:"port,attr,omitempty"`
      Local   *DomainInterfaceSourceLocal `xml:"local"`
  }

and (un)marshaling is done like

  func (a *DomainInterfaceSource) MarshalXML(e *xml.Encoder, start
xml.StartElement) error {
      if a.User != nil {
          return nil
      } else if a.Ethernet != nil {
          if len(a.Ethernet.IP) > 0 && len(a.Ethernet.Route) > 0 {
              return e.EncodeElement(a.Ethernet, start)
          }
          return nil
      } else if // ...
  }

  func (a *DomainInterfaceSource) UnmarshalXML(d *xml.Decoder, start
xml.StartElement) error {
      if a.User != nil {
          return d.DecodeElement(a.User, &start)
      } else if a.Ethernet != nil {
          return d.DecodeElement(a.Ethernet, &start)
      } else if // ...
  }

When using these data structures, you'll then do something like

  iface := DomainInterface{
      Source: &DomainInterfaceSource{
          Network: &DomainInterfaceNetwork{
              Network: "default",
          },
      },
  }

instead of

  iface := DomainInterface{
      Source: DomainInterfaceSource{
          Type:  "network",
          Value: DomainInterfaceNetwork{
              Network: "default",
          },
      },
  }

which is more compact and, crucially, allows the compiler to catch
many typing issues ahead of time. I think this approach would work
really well for QAPI too.

-- 
Andrea Bolognani / Red Hat / Virtualization