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

[Victor Toso]

Hi,

On Mon, Aug 29, 2022 at 01:27:06PM +0200, Markus Armbruster wrote:
> 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.

Ah, I've missed this bit. Thank you, it does make it much
simpler.

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

Ack.

Cheers,
Victor

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

signature.asc
Description: PGP signature