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

[Andrea Bolognani <abologna@redhat.com>]

On Wed, Aug 17, 2022 at 04:04:19PM +0200, Victor Toso wrote:
> On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> > > 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.

IIUC your concern is that the QAPI schema could gain a new type,
TotallyNotBlockdevOptions, which looks exactly like BlockdevOptions
except for one or more extra fields.

If QEMU then produced a JSON like

  { "description": { /* a TotallyNotBlockdevOptions here */ } }

and we'd try to deserialize it with Go code like

  ref := BlockdevRef{}
  json.Unmarsal(&ref)

we'd end up mistakenly parsing the TotallyNotBlockdevOptions as a
valid BlockdevOptions, dropping the extra fields in the process.

Does that correctly describe the reason why you feel that the use of
StrictDecode is necessary?

If so, I respectfully disagree :)

If the client code is expecting a BlockdevRef as the return value of
a command and QEMU is producing something that is *not* a BlockdevRef
instead, that's an obvious bug in QEMU. If the client code is
expecting a BlockdevRef as the return value of a command that is
specified *not* to return a BlockdevRef, that's an obvious bug in the
client code.

In neither case it should be the responsibility of the SDK to
second-guess the declared intent, especially when it's perfectly
valid for a type to be extended in a backwards-compatible way by
adding fields to it.

-- 
Andrea Bolognani / Red Hat / Virtualization