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