Re: [RFC PATCH v1 2/8] qapi: golang: Generate qapi's alternate types in Go

[Victor Toso]

Hi,

On Tue, May 10, 2022 at 11:10:45AM +0100, Daniel P. Berrangé wrote:
> On Sat, Apr 02, 2022 at 12:40:58AM +0200, Victor Toso wrote:
> > This patch handles QAPI alternate types and generates data
> > structures in Go that handles it.
> > 
> > At this moment, there are 5 alternates in qemu/qapi, they are:
> >  * BlockDirtyBitmapMergeSource
> >  * Qcow2OverlapChecks
> >  * BlockdevRef
> >  * BlockdevRefOrNull
> >  * StrOrNull
> > 
> > Alternate types are similar to Union but without a
> > discriminator that can be used to identify the underlying
> > value on the wire. It is needed to infer it. That can't be
> > easily mapped in Go.
> 
> I don't buy that. Given this example:
> 
>   type BlockdevRef struct {
>         // Options are:
>         // * definition (BlockdevOptions): defines a new block device inline
>         // * reference (string): references the ID of an existing block device
>         Value Any
>   }
> 
> What is the problem with having this Go struct:
> 
>   type BlockdevRef struct {
>         Definition *BlockdevOptions
>             Reference  *string
>   }

... this is better.

> when deserializing from JSON, we know exactly which one of
> these two fields to populate. The programmer consuming this can
> look at which field is non-nil.
> 
> When serializing to JSON, we serialize which ever field is
> non-nil.
> 
> If both fields are non-nil that's a programmer bug. Either
> ignore it and only serialize the first non-nil field, or raise
> an error.

It would be a programmer bug if they set a Value of a type not
allowed by Type's spec, but it would be a *runtime* error. Your
suggestion is more type safe.

Thanks.

> 
> > 
> > For each Alternate type, we will be using a Any type to hold the
> > value. 'Any' is an alias type for interface{} (similar to void* in C).
> > 
> > Similarly to the Enum types (see previous commit), we will implement
> > Marshaler and Unmarshaler interfaces for the Alternate types and in
> > those MarshalJSON() and UnmarshalJSON() methods is where we are going
> > to put the logic to read/set alternate's value.
> > 
> > Note that on UnmarshalJSON(), a helper function called StrictDecode()
> > will be used. This function is the main logic to infer if a given JSON
> > object fits in a given Go struct. Because we only have 5 alternate
> > types, it is not hard to validate the unmarshaling logic but we might
> > need to improve it in the future if Alternate with branches that have
> > similar fields appear.
> > 
> > Examples:
> >  * BlockdevRef
> > ```go
> >     // Data to set in BlockdevOptions
> >     qcow2 := BlockdevOptionsQcow2{}
> >     // BlockdevRef using a string
> >     qcow2.File = BlockdevRef{Value: "/some/place/my-image"}
> >     opt := BlockdevOptions{}
> >     opt.Driver = BlockdevDriverQcow2
> >     opt.Value = qcow2
> > 
> >     b, _ := json.Marshal(data.s)
> >     // string(b) == `{"driver":"qcow2","file":"/some/place/my-image"}`
> > ```
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  scripts/qapi/golang.py | 157 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 155 insertions(+), 2 deletions(-)
> 
> 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 :|
>

signature.asc
Description: PGP signature