Sorry it took me a while to find the time to look into this! Overall this second iteration is a significant improvement over the initial one. There are still a few things that I think should be changed, so for the time being I'm going to comment mostly on the generated Go code and leave the details of the implementation for later.
On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote: > This patch handles QAPI alternate types and generates data structures > in Go that handles it. > > At this moment, there are 5 alternates in qemu/qapi, they are: > * BlockDirtyBitmapMergeSource > * Qcow2OverlapChecks > * BlockdevRef > * BlockdevRefOrNull > * StrOrNull I personally don't think it's very useful to list all the alternate types in the commit message, or even mention how many there are. You do this for all other types too, and it seems to me that it's just an opportunity for incosistent information to make their way to the git repository - what if a new type is introduced between the time your series is queued and merged? I'd say just drop this part. > Example: > > qapi: > | { 'alternate': 'BlockdevRef', > | 'data': { 'definition': 'BlockdevOptions', > | 'reference': 'str' } } > > go: > | type BlockdevRef struct { > | Definition *BlockdevOptions > | Reference *string > | } > > usage: > | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}` > | k := BlockdevRef{} > | err := json.Unmarshal([]byte(input), &k) > | if err != nil { > | panic(err) > | } > | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image" Let me just say that I absolutely *love* how you've added these bits comparing the QAPI / Go representations as well as usage. They really help a lot understanding what the generator is trying to achieve :) > +// Creates a decoder that errors on unknown Fields > +// Returns true if successfully decoded @from string @into type > +// Returns false without error is failed with "unknown field" > +// Returns false with error is a different error was found > +func StrictDecode(into interface{}, from []byte) error { > + dec := json.NewDecoder(strings.NewReader(string(from))) > + dec.DisallowUnknownFields() > + > + if err := dec.Decode(into); err != nil { > + return err > + } > + return nil > +} The documentation doesn't seem to be consistent with how the function actually works: AFAICT it returns false *with an error* for any failure, including those caused by unknown fields being present in the input JSON. Looking at the generated code: > type BlockdevRef struct { > Definition *BlockdevOptions > Reference *string > } > > func (s BlockdevRef) MarshalJSON() ([]byte, error) { > if s.Definition != nil { > return json.Marshal(s.Definition) > } else if s.Reference != nil { > return json.Marshal(s.Reference) > } > return nil, errors.New("BlockdevRef has empty fields") Returning an error if no field is set is good. Can we be more strict and returning one if more than one is set as well? That feels better than just picking the first one. > 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? 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. > s.Definition = nil > } > // Check for string > { > s.Reference = new(string) > if err := StrictDecode(s.Reference, data); err == nil { > return nil > } > s.Reference = nil > } > > return errors.New(fmt.Sprintf("Can't convert to BlockdevRef: %s", > string(data))) On a similar note to the MarshalJSON comment above, I'm not sure this is right. If we find that more than one field of the alternate is set, we should error out - that's just invalid JSON we're dealing with. But if we couldn't find any, that might be valid JSON that's been produced by a version of QEMU that introduced additional options to the alternate. In the spirit of "be liberal in what you accept", I think we should probably not reject that? Of course then the client code will have to look like if r.Definition != nil { // do something with r.Definition } else if r.Reference != nil { // do something with r.Reference } else { // we don't recognize this - error out } but the same is going to be true for enums, events and everything else that can be extended in a backwards-compatible fashion. -- Andrea Bolognani / Red Hat / Virtualization