Re: [PATCH v2 11/11] docs: add notes on Golang code generator

[Victor Toso]

Hi,

On Wed, Oct 18, 2023 at 01:47:56PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
> 
> > The goal of this patch is converge discussions into a documentation,
> > to make it easy and explicit design decisions, known issues and what
> > else might help a person interested in how the Go module is generated.
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  docs/devel/index-build.rst          |   1 +
> >  docs/devel/qapi-golang-code-gen.rst | 376 ++++++++++++++++++++++++++++
> >  2 files changed, 377 insertions(+)
> >  create mode 100644 docs/devel/qapi-golang-code-gen.rst
> >
> > diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
> > index 57e8d39d98..8f7c6f5dc7 100644
> > --- a/docs/devel/index-build.rst
> > +++ b/docs/devel/index-build.rst
> > @@ -15,5 +15,6 @@ the basics if you are adding new files and targets to the 
> > build.
> >     qtest
> >     ci
> >     qapi-code-gen
> > +   qapi-golang-code-gen
> >     fuzzing
> >     control-flow-integrity
> 
> Let's not worry whether and how this should be integrated with
> qapi-code-gen.rst for now.
> 
> I'm a Go ignoramus.  I hope my comments are at least somewhat
> helpful all the same.

They always are.

> > diff --git a/docs/devel/qapi-golang-code-gen.rst 
> > b/docs/devel/qapi-golang-code-gen.rst
> > new file mode 100644
> > index 0000000000..b62daf3bad
> > --- /dev/null
> > +++ b/docs/devel/qapi-golang-code-gen.rst
> > @@ -0,0 +1,376 @@
> > +==========================
> > +QAPI Golang code generator
> > +==========================
> > +
> > +..
> > +   Copyright (C) 2023 Red Hat, Inc.
> > +
> > +   This work is licensed under the terms of the GNU GPL, version 2 or
> > +   later.  See the COPYING file in the top-level directory.
> > +
> > +
> > +Introduction
> > +============
> > +
> > +This document provides information of how the generated Go code maps
> > +with the QAPI specification, clarifying design decisions when needed.
> > +
> > +
> > +Scope of the generated Go code
> > +==============================
> 
> What do you mean by "scope"?

To build an application to talk with QEMU over QMP, this
generated code is not enough. What I mean is that, this is just
the first layer. We still need a library on top to do the work of
connecting, sending/receiving messages, etc.

Any recommendations on how to word this better?

> > +
> > +The scope is limited to data structures that can interpret and be used
> > +to generate valid QMP messages. These data structures are generated
> > +from a QAPI schema and should be able to handle QMP messages from the
> > +same schema.
> > +
> > +The generated Go code is a Go module with data structs that uses Go
> > +standard library ``encoding/json``, implementing its field tags and
> > +Marshal interface whenever needed.
> > +
> > +
> > +QAPI types to Go structs
> > +========================
> > +
> > +Enum
> > +----
> > +
> > +Enums are mapped as strings in Go, using a specified string type per
> > +Enum to help with type safety in the Go application.
> > +
> > +::
> > +
> > +    { 'enum': 'HostMemPolicy',
> > +      'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
> > +
> > +.. code-block:: go
> > +
> > +    type HostMemPolicy string
> > +
> > +    const (
> > +        HostMemPolicyDefault    HostMemPolicy = "default"
> > +        HostMemPolicyPreferred  HostMemPolicy = "preferred"
> > +        HostMemPolicyBind       HostMemPolicy = "bind"
> > +        HostMemPolicyInterleave HostMemPolicy = "interleave"
> > +    )
> > +
> > +
> > +Struct
> > +------
> > +
> > +The mapping between a QAPI struct in Go struct is very straightforward.
> > + - Each member of the QAPI struct has its own field in a Go struct.
> > + - Optional members are pointers type with 'omitempty' field tag set
> > +
> > +One important design decision was to _not_ embed base struct, copying
> > +the base members to the original struct. This reduces the complexity
> > +for the Go application.
> > +
> > +::
> > +
> > +    { 'struct': 'BlockExportOptionsNbdBase',
> > +      'data': { '*name': 'str', '*description': 'str' } }
> > +
> > +    { 'struct': 'BlockExportOptionsNbd',
> > +      'base': 'BlockExportOptionsNbdBase',
> > +      'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
> > +                '*allocation-depth': 'bool' } }
> > +
> > +.. code-block:: go
> > +
> > +    type BlockExportOptionsNbd struct {
> > +        Name        *string `json:"name,omitempty"`
> > +        Description *string `json:"description,omitempty"`
> > +
> > +        Bitmaps         []BlockDirtyBitmapOrStr `json:"bitmaps,omitempty"`
> > +        AllocationDepth *bool                   
> > `json:"allocation-depth,omitempty"`
> > +    }
> 
> Is there a need to convert from type to base type?

Do you mean why we don't embed a base struct and do a copy of its
fields instead? If yes, the main reason is aesthetic. See this
issue: https://github.com/golang/go/issues/29438

So, we could have a situation where we have a embed type, inside
a embed type, inside of embed type making it horrible to write
the code for using it.
 
> In C, we get away with (BlockExportOptionsNbdBase *)obj.  We hide it in
> an inline function, though.
> 
> > +
> > +
> > +Union
> > +-----
> > +
> > +Unions in QAPI are binded to a Enum type which provides all possible
> 
> bound to an Enum

Fixed.

> > +branches of the union. The most important caveat here is that the Union
> > +does not need to have a complex type implemented for all possible
> > +branches of the Enum. Receiving a enum value of a unimplemented branch
> 
> I'd call that branch empty, not unimplemented.

Ok, changed them all to empty branch.

> > +is valid.
> > +
> > +For this reason, the generated Go struct will define a field for each
> > +Enum value. The Go type defined for unbranched Enum values is bool
> 
> Important design decision: since Go sucks at sum types, you elect to add
> all the variant members to the struct.  Only one of them may be used at
> any time.  Please spell that out more clearly.

What about:

    The generated Go struct will then define a field for each
    Enum value. The type for Enum values of empty branch is bool.
    Only one field can be set at time.

> > +
> > +Go struct and also implement the Marshal interface.
> 
> Blank line in the middle of a sentence?  Or two sentences?  Can't make
> sense of them.

Leftover when rewriting it. Removed it.

> What do you mean by "unbranched" Enum value?  Enum value with an
> implicit (empty) branch?

Yes, changing it to empty branch or empty branched.

> > +
> > +As each Union Go struct type has both the discriminator field and
> > +optional fields, it is important to note that when converting Go struct
> > +to JSON, we only consider the discriminator field if no optional field
> > +member was set. In practice, the user should use the optional fields if
> > +the QAPI Union type has defined them, otherwise the user can set the
> > +discriminator field for the unbranched enum value.
> 
> I don't think I get this paragraph.

Sorry, leftover again. I've removed it entirely.

This bit was when we had a Discriminator field, we don't have it
anymore.

> > +
> > +::
> > +
> > +    { 'union': 'ImageInfoSpecificQCow2Encryption',
> > +      'base': 'ImageInfoSpecificQCow2EncryptionBase',
> > +      'discriminator': 'format',
> > +      'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> 
> The example is hard to understand without additional context, namely:
> 
>        { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
>          'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
> 
>        { 'enum': 'BlockdevQcow2EncryptionFormat',
>          'data': [ 'aes', 'luks' ] }

Added.

> > +
> > +.. code-block:: go
> > +
> > +    type ImageInfoSpecificQCow2Encryption struct {
> > +        // Variants fields
> > +        Luks *QCryptoBlockInfoLUKS `json:"-"`
> > +        // Unbranched enum fields
> > +        Aes bool `json:"-"`
> > +    }
> 
> The members of the base type are the common members, and the members of
> the branch's type are the variant members.
> 
> Your example shows the variant member: 'luks'.
> 
> The common member @format isn't there.  I guess you're eliding it
> because you can derive its value from other members.

Correct. We can define @format value based on user's setting Aes
or Luks.

> If there were other common members, where would they go?

They should all be at the top of the struct, for example:

    { 'union': 'ExpirePasswordOptions',
      'base': { 'protocol': 'DisplayProtocol',
                'time': 'str' },
      'discriminator': 'protocol',
      'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }

    { 'enum': 'DisplayProtocol',
      'data': [ 'vnc', 'spice' ] }

generates:

    type ExpirePasswordOptions struct {
        Time string `json:"time"`
        // Variants fields
        Vnc *ExpirePasswordOptionsVnc `json:"-"`
        // Unbranched enum fields
        Spice bool `json:"-"`
    }

if you want to navigate over it:

    
https://gitlab.com/victortoso/qapi-go/-/blob/qapi-golang-v2-by-tags/pkg/qapi/unions.go?ref_type=heads#L4516

> > +
> > +    func (s ImageInfoSpecificQCow2Encryption) MarshalJSON() ([]byte, 
> > error) {
> > +        // ...
> > +        // Logic for branched Enum
> > +        if s.Luks != nil && err == nil {
> > +            if len(bytes) != 0 {
> > +                err = errors.New(`multiple variant fields set`)
> > +            } else if err = unwrapToMap(m, s.Luks); err == nil {
> > +                m["format"] = BlockdevQcow2EncryptionFormatLuks
> > +                bytes, err = json.Marshal(m)
> > +            }
> > +        }
> > +
> > +        // Logic for unbranched Enum
> > +        if s.Aes && err == nil {
> > +            if len(bytes) != 0 {
> > +                err = errors.New(`multiple variant fields set`)
> > +            } else {
> > +                m["format"] = BlockdevQcow2EncryptionFormatAes
> > +                bytes, err = json.Marshal(m)
> > +            }
> > +        }
> > +
> > +        // ...
> > +        // Handle errors
> > +    }
> > +
> > +
> > +    func (s *ImageInfoSpecificQCow2Encryption) UnmarshalJSON(data []byte) 
> > error {
> > +        // ...
> > +
> > +        switch tmp.Format {
> > +        case BlockdevQcow2EncryptionFormatLuks:
> > +            s.Luks = new(QCryptoBlockInfoLUKS)
> > +            if err := json.Unmarshal(data, s.Luks); err != nil {
> > +                s.Luks = nil
> > +                return err
> > +            }
> > +        case BlockdevQcow2EncryptionFormatAes:
> > +            s.Aes = true
> > +
> > +        default:
> > +            return fmt.Errorf("error: unmarshal: 
> > ImageInfoSpecificQCow2Encryption: received unrecognized value: '%s'",
> > +                tmp.Format)
> > +        }
> > +        return nil
> > +    }
> > +
> > +
> > +Alternate
> > +---------
> > +
> > +Like Unions, alternates can have a few branches. Unlike Unions, they
> 
> Scratch "a few".

Fixed
 
> > +don't have a discriminator field and each branch should be a different
> > +class of Type entirely (e.g: You can't have two branches of type int in
> > +one Alternate).
> > +
> > +While the marshalling is similar to Unions, the unmarshalling uses a
> > +try-and-error approach, trying to fit the data payload in one of the
> > +Alternate fields.
> > +
> > +The biggest caveat is handling Alternates that can take JSON Null as
> > +value. The issue lies on ``encoding/json`` library limitation where
> > +unmarshalling JSON Null data to a Go struct which has the 'omitempty'
> > +field that, it bypass the Marshal interface. The same happens when
> > +marshalling, if the field tag 'omitempty' is used, a nil pointer would
> > +never be translated to null JSON value.
> > +
> > +The problem being, we use pointer to type plus ``omitempty`` field to
> > +express a QAPI optional member.
> > +
> > +In order to handle JSON Null, the generator needs to do the following:
> > +  - Read the QAPI schema prior to generate any code and cache
> > +    all alternate types that can take JSON Null
> > +  - For all Go structs that should be considered optional and they type
> > +    are one of those alternates, do not set ``omitempty`` and implement
> > +    Marshal interface for this Go struct, to properly handle JSON Null
> > +  - In the Alternate, uses a boolean 'IsNull' to express a JSON Null
> > +    and implement the AbsentAlternate interface, to help sturcts know
> 
> Typo: to help structs

Thanks
 
> > +    if a given Alternate type should be considered Absent (not set) or
> > +    any other possible Value, including JSON Null.
> > +
> > +::
> > +
> > +    { 'alternate': 'BlockdevRefOrNull',
> > +      'data': { 'definition': 'BlockdevOptions',
> > +                'reference': 'str',
> > +                'null': 'null' } }
> > +
> > +.. code-block:: go
> > +
> > +    type BlockdevRefOrNull struct {
> > +        Definition *BlockdevOptions
> > +        Reference  *string
> > +        IsNull     bool
> > +    }
> > +
> > +    func (s *BlockdevRefOrNull) ToAnyOrAbsent() (any, bool) {
> > +        if s != nil {
> > +            if s.IsNull {
> > +                return nil, false
> > +            } else if s.Definition != nil {
> > +                return *s.Definition, false
> > +            } else if s.Reference != nil {
> > +                return *s.Reference, false
> > +            }
> > +        }
> > +
> > +        return nil, true
> > +    }
> > +
> > +    func (s BlockdevRefOrNull) MarshalJSON() ([]byte, error) {
> > +        if s.IsNull {
> > +            return []byte("null"), nil
> > +        } else if s.Definition != nil {
> > +            return json.Marshal(s.Definition)
> > +        } else if s.Reference != nil {
> > +            return json.Marshal(s.Reference)
> > +        }
> > +        return []byte("{}"), nil
> > +    }
> > +
> > +    func (s *BlockdevRefOrNull) UnmarshalJSON(data []byte) error {
> > +        // Check for json-null first
> > +        if string(data) == "null" {
> > +            s.IsNull = true
> > +            return nil
> > +        }
> > +        // Check for BlockdevOptions
> > +        {
> > +            s.Definition = new(BlockdevOptions)
> > +            if err := StrictDecode(s.Definition, data); err == nil {
> > +                return nil
> > +            }
> > +            s.Definition = nil
> > +        }
> > +        // Check for string
> > +        {
> > +            s.Reference = new(string)
> > +            if err := StrictDecode(s.Reference, data); err == nil {
> > +                return nil
> > +            }
> > +            s.Reference = nil
> > +        }
> > +
> > +        return fmt.Errorf("Can't convert to BlockdevRefOrNull: %s", 
> > string(data))
> > +    }
> > +
> > +
> > +Event
> > +-----
> > +
> > +All events are mapped to its own struct with the additional
> 
> Each event is mapped to its own

Fixed

> > +MessageTimestamp field, for the over-the-wire 'timestamp' value.
> > +
> > +Marshaling and Unmarshaling happens over the Event interface, so users
> > +should use the MarshalEvent() and UnmarshalEvent() methods.
> > +
> > +::
> > +
> > +    { 'event': 'SHUTDOWN',
> > +      'data': { 'guest': 'bool',
> > +                'reason': 'ShutdownCause' } }
> > +
> > +.. code-block:: go
> > +
> > +    type Event interface {
> > +        GetName() string
> > +        GetTimestamp() Timestamp
> > +    }
> > +
> > +    type ShutdownEvent struct {
> > +        MessageTimestamp Timestamp     `json:"-"`
> > +        Guest            bool          `json:"guest"`
> > +        Reason           ShutdownCause `json:"reason"`
> > +    }
> > +
> > +    func (s *ShutdownEvent) GetName() string {
> > +        return "SHUTDOWN"
> > +    }
> > +
> > +    func (s *ShutdownEvent) GetTimestamp() Timestamp {
> > +        return s.MessageTimestamp
> > +    }
> > +
> > +
> > +Command
> > +-------
> > +
> > +All commands are mapped to its own struct with the additional MessageId
> 
> Each command is mapped to its own

Fixed
 
> > +field for the optional 'id'. If the command has a boxed data struct,
> > +the option struct will be embed in the command struct.
> > +
> > +As commands do require a return value, every command has its own return
> > +type. The Command interface has a GetReturnType() method that returns a
> > +CommandReturn interface, to help Go application handling the data.
> > +
> > +Marshaling and Unmarshaling happens over the Command interface, so
> > +users should use the MarshalCommand() and UnmarshalCommand() methods.
> > +
> > +::
> > +
> > +   { 'command': 'set_password',
> > +     'boxed': true,
> > +     'data': 'SetPasswordOptions' }
> 
> Since you show the Go type generated for QAPI type SetPasswordOptions,
> you should show the QAPI type here.

Added.
             
> > +
> > +.. code-block:: go
> > +
> > +    type Command interface {
> > +        GetId() string
> > +        GetName() string
> > +        GetReturnType() CommandReturn
> > +    }
> > +
> > +    // SetPasswordOptions is embed
> > +    type SetPasswordCommand struct {
> > +        SetPasswordOptions
> > +        MessageId string `json:"-"`
> > +    }
> > +
> > +    // This is an union
> > +    type SetPasswordOptions struct {
> > +        Protocol  DisplayProtocol    `json:"protocol"`
> > +        Password  string             `json:"password"`
> > +        Connected *SetPasswordAction `json:"connected,omitempty"`
> > +
> > +        // Variants fields
> > +        Vnc *SetPasswordOptionsVnc `json:"-"`
> > +    }
> > +
> > +Now an example of a command without boxed type.
> > +
> > +::
> > +
> > +    { 'command': 'set_link',
> > +      'data': {'name': 'str', 'up': 'bool'} }
> > +
> > +.. code-block:: go
> > +
> > +    type SetLinkCommand struct {
> > +        MessageId string `json:"-"`
> > +        Name      string `json:"name"`
> > +        Up        bool   `json:"up"`
> > +    }
> > +
> > +Known issues
> > +============
> > +
> > +- Type names might not follow proper Go convention. Andrea suggested an
> > +  annotation to the QAPI schema that could solve it.
> > +  https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg00127.html

Many thanks for the review,
Victor

signature.asc
Description: PGP signature