Hi, On Wed, Oct 18, 2023 at 01:47:56PM +0200, Markus Armbruster wrote: > Victor Toso <victort...@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 <victort...@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
