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

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

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.

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

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

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

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

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

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

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

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

> +
> +::
> +
> +    { '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' ] }

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

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

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

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

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

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

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

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

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

Victor Toso <victortoso@redhat.com> writes:

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

Thanks!

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

Yes: try working your explanation of layers into the text.

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

Yes, avoiding such horrors is why we copy the base type's members
generated C.

In object oriented languages with inheritance, you can accomplish the
same by inheriting the base type.  These languages commonly let you
convert to the base type then.

When copying the base type's members, converting to the base type may
require extra code.  It doesn't in C:

>> In C, we get away with (BlockExportOptionsNbdBase *)obj.  We hide it in
>> an inline function, though.

I don't actually know whether it does in Go.  Only matters if there's an
actual need for such a conversion, hence my question.

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

Better.

We'll polish the prose later.

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

Recommend to use an example that lets us show Go for such common members.

[...]

>
> Many thanks for the review,
> Victor

You're welcome!