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

[Victor Toso]

Hi,

On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> Sorry it took me a while to find the time to look into this!

(1.5 month later.. what can I say!) :)

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

Sure, and thanks.

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

No issue on my side in dropping this bits.

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

Thanks!

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

You are correct, documentation of this helper function needs to
be fixed if we keep the helper function, as you made a good point
about backwards-compatible decoding a struct that might have
introduced extra fields in a newer version.

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

Sure.

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

That's correct. This means that with this RFCv2 proposal, qapi-go
based on qemu version 7.1 might not be able to decode a qmp
message from qemu version 7.2 if it has introduced a new field.

This needs fixing, not sure yet the way to go.

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

StrictDecode is only used with alternates because, unlike unions,
Alternate types don't have a 'discriminator' field that would
allow us to know what data type to expect.

With this in mind, theoretically speaking, we could have very
similar struct types as Alternate fields and we have to find on
runtime which type is that underlying byte stream.

So, to reply to your suggestion, if we allow BlockdevRef without
StrictDecode we might find ourselves in a situation that it
matched a few fields of BlockdevOptions but it the byte stream
was actually another type.

So, I acknowledge that current version of this patch is not
correct but we still need to look into the fields and see if
there is a perfect match with the information that we have.

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

With StrictDecode (or something similar) there shouldn't be
multiple fields being set on the Go structure. Once it finds a
match, it returns nil (no error)

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

The client code looking like above is not much different than

    if err := json.Unmarshal([]byte(input), &r); err != nil {
        // error out: bad json? incompatibility issue?
    } else if r.Definition != nil {
        // do something with r.Definition
    } else if r.Reference != nil {
        // do something with r.Reference
    } else {
        // empty might be valid though, e.g: JSON null
    }

My main concern with alternate logic is not putting data that
should go to r.Reference into r.Definition and vice versa.

I took note of all your suggestions, I'll be reworking this. I'll
revisit the problem with StrictDecode together with addressing my
reply to Daniel:

    https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg03140.html

Cheers,
Victor

signature.asc
Description: PGP signature