qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Victor Toso
Subject: Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
Date: Mon, 9 May 2022 12:21:10 +0200

Hi!

Sorry for taking some time to reply.

On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> 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.

Glad to hear.

> 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.

There was a good discussion around this with Markus so I don't
want to break it in another thread.

I'm happy that you have found those inconsistencies. I'll reply
on the other thread about it but I don't mind working towards
fixing it, either at code generator level or at QAPI level.

> 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:

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.

> 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.

I completely agree with the message below: Type validation is
important at this stage.

> 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

That isn't too hard to implement and I've started with this
design at first. Each QAPI Command can implement a method Name()
which returns the over-the-wire name for that Command.

I'm not yet sure if this is preferable over some other syntactic
sugar function that might be generated (this series) or the next
layer that will be on top of this.

But I agree with you that it should be improved before reaching
actual Applications.

>   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'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.

Agree. Many thanks again, for the review, suggestions and
discussions.

Cheers,
Victor

> -- 
> 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)
> }

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]