Re: [RFC PATCH v2 3/8] qapi: golang: Generate qapi's struct types in Go

[Victor Toso <victortoso@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4126 bytes --]

Hi,

On Fri, Jun 17, 2022 at 03:41:10PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 17, 2022 at 02:19:27PM +0200, Victor Toso wrote:
> > This patch handles QAPI struct types and generates the equivalent
> > types in Go.
> > 
> > At the time of this writing, it generates 388 structures.
> > 
> > The highlights of this implementation are:
> > 
> > 1. Generating an Go struct that requires a @base type, the @base type
> >    fields are copied over to the Go struct. The advantage of this
> >    approach is to not have embed structs in any of the QAPI types.
> >    The downside are some generated Types that are likely useless now,
> >    like InetSocketAddressBase from InetSocketAddress.
> > 
> > 2. About the Go struct's fields:
> > 
> >   i) They can be either by Value or Reference.
> > 
> >   ii) Every field that is marked as optional in the QAPI specification
> >   are translated to Reference fields in its Go structure. This design
> >   decision is the most straightforward way to check if a given field
> >   was set or not.
> > 
> >   iii) Mandatory fields are always by Value with the exception of QAPI
> >   arrays, which are handled by Reference (to a block of memory) by Go.
> > 
> >   iv) All the fields are named with Uppercase due Golang's export
> >   convention.
> > 
> >   v) In order to avoid any kind of issues when encoding ordecoding, to
> >   or from JSON, we mark all fields with its @name and, when it is
> >   optional, member, with @omitempty
> > 
> > Example:
> > 
> > qapi:
> >   | { 'struct': 'BlockdevCreateOptionsFile',
> >   |   'data': { 'filename':             'str',
> >   |             'size':                 'size',
> >   |             '*preallocation':       'PreallocMode',
> >   |             '*nocow':               'bool',
> >   |             '*extent-size-hint':    'size'} }
> > 
> > go:
> >   | type BlockdevCreateOptionsFile struct {
> >   |         Filename       string        `json:"filename"`
> >   |         Size           uint64        `json:"size"`
> >   |         Preallocation  *PreallocMode `json:"preallocation,omitempty"`
> >   |         Nocow          *bool         `json:"nocow,omitempty"`
> >   |         ExtentSizeHint *uint64       `json:"extent-size-hint,omitempty"`
> >   | }
> 
> One thing to bear in mind here
> 
> At the QAPI level, changing a field from mandatory to optional has
> been considered a backwards compatible change by QEMU maintainers,
> because any existing caller can happily continue passing the
> optional field with no downside.
> 
> With this Go design, changing a field from mandatory to optional
> will be an API breakage, because the developer will need to change
> from passing a literal value, to a pointer to the value, when
> initializing the struct.
> 
> IOW, this Go impl provides weaker compat guarantees than even
> QAPI does, and QAPI compat guarantees were already weaker than
> I would like as an app developer.

I think the current draft should be considered an interface that
can work with the QEMU version this was generated from. That is
the first thing we should get right.

> If we want to make ourselves future proof, we would have to
> make all struct fields optional from the start, even if they
> are mandatory at QAPI level. This would make the code less
> self-documenting though, so that's not very appealing either.
 
> If we want to avoid this, we would need the same approach I
> suggested wrt support multiple versions of the API
> concurrently. Namely have versioned structs, so every time
> there's a field change of any kind, we introduce a new struct
> version.

That's more or less what I had in mind. I mentioned it in the
item 8 of the cover-letter. I just did not want to address it at
before deciding what the structs should look like first, for the
version we are generating from.

Just to clarify, so far I plan to follow the suggestion:
    https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg02147.html

Of course, If there are other ideas, we can discuss it too.

Cheers,
Victor

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]