Re: [PATCH v1 5/9] qapi: golang: Generate qapi's union types in Go

[Daniel P . Berrangé]

On Wed, Sep 27, 2023 at 01:25:40PM +0200, Victor Toso wrote:
> This patch handles QAPI union types and generates the equivalent data
> structures and methods in Go to handle it.
> 
> The QAPI union type has two types of fields: The @base and the
> @Variants members. The @base fields can be considered common members
> for the union while only one field maximum is set for the @Variants.
> 
> In the QAPI specification, it defines a @discriminator field, which is
> an Enum type. The purpose of the  @discriminator is to identify which
> @variant type is being used.
> 
> Not that @discriminator's enum might have more values than the union's
> data struct. This is fine. The union does not need to handle all cases
> of the enum, but it should accept them without error. For this
> specific case, we keep the @discriminator field in every union type.

I still tend think the @discriminator field should not be
present in the union structs. It feels like we're just trying
to directly copy the C code in Go and so smells wrong from a
Go POV.

For most of the unions the @discriminator field will be entirely
redundant, becasue the commonm case is that a @variant field
exists for every possible @discriminator value.

To take one example

  type SocketAddress struct {
        Type SocketAddressType `json:"type"`

        // Variants fields
        Inet  *InetSocketAddress  `json:"-"`
        Unix  *UnixSocketAddress  `json:"-"`
        Vsock *VsockSocketAddress `json:"-"`
        Fd    *String             `json:"-"`
  }

If one was just writing Go code without the pre-existing knowledge
of the QAPI C code, 'Type' is not something a Go programmer would
be inclined add IMHO.

And yet you are right that we need a way to represent a @discriminator
value that has no corresponding @variant, since QAPI allows for that
scenario. To deal with that I would suggest we just use an empty
interface type. eg


  type SocketAddress struct {
        Type SocketAddressType `json:"type"`

        // Variants fields
        Inet  *InetSocketAddress  `json:"-"`
        Unix  *UnixSocketAddress  `json:"-"`
        Vsock *VsockSocketAddress `json:"-"`
        Fd    *String             `json:"-"`
        Fish  *interface{}        `json:"-"`
        Food  *interface()        `json:"-"`
  }

the pointer value for 'Fish' and 'Food' fields here merely needs to
be non-NULL, it doesn't matter what the actual thing assigned is.


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