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

[Victor Toso <victortoso@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5414 bytes --]

Hi,

On Fri, Aug 19, 2022 at 11:27:13AM -0500, Andrea Bolognani wrote:
> 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?

Not quite. The problem here is related to the Alternate types of
the QAPI specification [0], just to name a simple in-use example,
BlockdevRefOrNul [1].

[0] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/qapi-code-gen.rst?plain=1#L387
[1] https://gitlab.com/qemu-project/qemu/-/blob/master/qapi/block-core.json#L4349

To exemplify the problem that I try to solve with StrictDecode,
let's say there is a DeviceRef alternate type that looks like:

{ 'alternate': 'DeviceRef',
  'data': { 'memory': 'BlockdevRefInMemory',
            'disk': 'BlockdevRefInDisk',
            'cloud': 'BlockdevRefInCloud' } }

Just a quick recap, at runtime we don't have data's payload name
(e.g: disk).  We need to check the actual data and try to find
what is the payload type.

type BlockdevRefInMemory struct {
    Name  *string
    Size  uint64
    Start uint64
    End   uint64
}

type BlockdevRefInDisk struct {
    Name  *string
    Size  uint64
    Path  *string
}

type BlockdevRefInCloud struct {
    Name  *string
    Size  uint64
    Uri   *string
}

All types have unique fields but they all share some fields too.
Golang, without StrictDecode would happily decode a "disk"
payload into either "memory" or "cloud" fields, matching only
what the json provides and ignoring the rest. StrictDecode would
error if the payload had fields that don't belong to that Type so
we could try to find a perfect match.

While this should work reliably with current version of QEMU's
qapi-schema.json, it is not future error proof... It is just a
bit better than not checking at all.

The overall expectations is that, if the fields have too much in
common, it is likely they should have been tagged as 'union'
instead of 'alternate'. Yet, because alternate types have this
flexibility, we need to be extra careful.

I'm still thinking about this in a way that would not give too
much hassle when considering a generated code that talks with
older/newer qemu where some fields might have been added/removed.

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

Yes, the SDK should consider valid QMP messages. This specific
case is just a bit more complex qapi type that SDK needs to
worry.

Thanks for your input!
Victor

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]