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

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

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

Quick intercession (I merely skimmed the review thread; forgive me if
it's not useful or not new):

    An alternate type is like a union type, except there is no
    discriminator on the wire.  Instead, the branch to use is inferred
    from the value.  An alternate can only express a choice between types
    represented differently on the wire.

This is docs/devel/qapi-code-gen.rst.  Implied there: the inference is
based on the JSON type *only*, i.e. no two branches can have the same
JSON type on the wire.  Since all complex types (struct or union) are
JSON object on the wire, at most one alternate branch can be of complex
type.

More sophisticated inference would be possible if we need it.  So far we
haven't.

> 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