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

[Daniel P . Berrangé]

On Tue, May 10, 2022 at 01:37:50PM -0400, Andrea Bolognani wrote:
> 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",
> > >       },
> > >   }

Note there is clear redundancy here between 'Name' and the struct
type. IMHO the better approach would be

       cmd := TraceEventGetStateCommand{
          Name: 'qemu_memalign'
       }

and have 'Command' simply as an interface that TraceEventGetStateCommand
implements. I don't think the interface would need more than a
Marshal and Demarshal method, which serialize the 'Arg' and then add
the Name field explicitly. 


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

The main reason not to flatten is if you have scenarios where it is
useful to work against the base type directly. I'm not sure that we
have such a need though.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|