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

[Andrea Bolognani]

Sorry it took me a while to find the time to look into this!

Overall this second iteration is a significant improvement over the
initial one. There are still a few things that I think should be
changed, so for the time being I'm going to comment mostly on the
generated Go code and leave the details of the implementation for
later.


On Fri, Jun 17, 2022 at 02:19:26PM +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

I personally don't think it's very useful to list all the alternate
types in the commit message, or even mention how many there are. You
do this for all other types too, and it seems to me that it's just an
opportunity for incosistent information to make their way to the git
repository - what if a new type is introduced between the time your
series is queued and merged? I'd say just drop this part.

> Example:
>
> qapi:
>   | { 'alternate': 'BlockdevRef',
>   |   'data': { 'definition': 'BlockdevOptions',
>   |             'reference': 'str' } }
>
> go:
>   | type BlockdevRef struct {
>   |         Definition *BlockdevOptions
>   |         Reference  *string
>   | }
>
> usage:
>   | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
>   | k := BlockdevRef{}
>   | err := json.Unmarshal([]byte(input), &k)
>   | if err != nil {
>   |     panic(err)
>   | }
>   | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"

Let me just say that I absolutely *love* how you've added these bits
comparing the QAPI / Go representations as well as usage. They really
help a lot understanding what the generator is trying to achieve :)

> +// Creates a decoder that errors on unknown Fields
> +// Returns true if successfully decoded @from string @into type
> +// Returns false without error is failed with "unknown field"
> +// Returns false with error is a different error was found
> +func StrictDecode(into interface{}, from []byte) error {
> +    dec := json.NewDecoder(strings.NewReader(string(from)))
> +    dec.DisallowUnknownFields()
> +
> +    if err := dec.Decode(into); err != nil {
> +        return err
> +    }
> +    return nil
> +}

The documentation doesn't seem to be consistent with how the function
actually works: AFAICT it returns false *with an error* for any
failure, including those caused by unknown fields being present in
the input JSON.


Looking at the generated code:

> type BlockdevRef struct {
>     Definition *BlockdevOptions
>     Reference  *string
> }
>
> func (s BlockdevRef) MarshalJSON() ([]byte, error) {
>     if s.Definition != nil {
>         return json.Marshal(s.Definition)
>     } else if s.Reference != nil {
>         return json.Marshal(s.Reference)
>     }
>     return nil, errors.New("BlockdevRef has empty fields")

Returning an error if no field is set is good. Can we be more strict
and returning one if more than one is set as well? That feels better
than just picking the first one.

> func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
>     // Check for json-null first
>     if string(data) == "null" {
>         return errors.New(`null not supported for BlockdevRef`)
>     }
>     // Check for BlockdevOptions
>     {
>         s.Definition = new(BlockdevOptions)
>         if err := StrictDecode(s.Definition, data); err == nil {
>             return nil
>         }

The use of StrictDecode() here means that we won't be able to parse
an alternate produced by a version of QEMU where BlockdevOptions has
gained additional fields, doesn't it?

Considering that we will happily parse such a BlockdevOptions outside
of the context of BlockdevRef, I think we should be consistent and
allow the same to happen here.

>         s.Definition = nil
>     }
>     // Check for string
>     {
>         s.Reference = new(string)
>         if err := StrictDecode(s.Reference, data); err == nil {
>             return nil
>         }
>         s.Reference = nil
>     }
>
>     return errors.New(fmt.Sprintf("Can't convert to BlockdevRef: %s", 
> string(data)))

On a similar note to the MarshalJSON comment above, I'm not sure this
is right.

If we find that more than one field of the alternate is set, we
should error out - that's just invalid JSON we're dealing with. But
if we couldn't find any, that might be valid JSON that's been
produced by a version of QEMU that introduced additional options to
the alternate. In the spirit of "be liberal in what you accept", I
think we should probably not reject that? Of course then the client
code will have to look like

  if r.Definition != nil {
      // do something with r.Definition
  } else if r.Reference != nil {
      // do something with r.Reference
  } else {
      // we don't recognize this - error out
  }

but the same is going to be true for enums, events and everything
else that can be extended in a backwards-compatible fashion.

-- 
Andrea Bolognani / Red Hat / Virtualization