Hi,
On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote:
> On Mon, Oct 16, 2023 at 05:27:00PM +0200, Victor Toso wrote:
> > This patch handles QAPI union types and generates the equivalent data
> > structures and methods in Go to handle it.
> >
> > The QAPI union type has two types of fields: The @base and the
> > @Variants members. The @base fields can be considered common members
> > for the union while only one field maximum is set for the @Variants.
> >
> > In the QAPI specification, it defines a @discriminator field, which is
> > an Enum type. The purpose of the @discriminator is to identify which
> > @variant type is being used.
> >
> > For the @discriminator's enum that are not handled by the QAPI Union,
> > we add in the Go struct a separate block as "Unbranched enum fields".
> > The rationale for this extra block is to allow the user to pass that
> > enum value under the discriminator, without extra payload.
> >
> > The union types implement the Marshaler and Unmarshaler interfaces to
> > seamless decode from JSON objects to Golang structs and vice versa.
> >
> > qapi:
> > | { 'union': 'SetPasswordOptions',
> > | 'base': { 'protocol': 'DisplayProtocol',
> > | 'password': 'str',
> > | '*connected': 'SetPasswordAction' },
> > | 'discriminator': 'protocol',
> > | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
> >
> > go:
> > | type SetPasswordOptions struct {
> > | Password string `json:"password"`
> > | Connected *SetPasswordAction `json:"connected,omitempty"`
> > | // Variants fields
> > | Vnc *SetPasswordOptionsVnc `json:"-"`
> > | // Unbranched enum fields
> > | Spice bool `json:"-"`
> > | }
>
> Instead of using bool for these, can we denote a special type? For
> example
>
> type Empty struct{}
>
> We could then do
>
> u := SetPasswordOptions{
> Password: "...",
> Spice: &Empty{},
> }
>
> The benefit I have in mind is that you'd be able to check which
> variant field is set consistently:
>
> if u.Vnc != nil {
> ...
> }
> if u.Spice != nil {
> ...
> }
>
> Additionally, this would allow client code that *looks* at the
> union to keep working even if actual data is later added to the
> branch; client code that *creates* the union would need to be
> updated, of course, but that would be the case regardless.I think it is better to not have code that is working to keep working in this case where Spice is implemented. Implementing Spice here would mean that a struct type SetPasswordOptionsSpice was created but because the code handling it before was using struct type Empty, it will not handle the new struct, leading to possible runtime errors (e.g: not handling username/password) A bool would be simpler, triggering compile time errors. Note that I'm working with the mindset that each version of the module talks well with a version of QEMU. We should consider next if we want to implement logic for QAPI versioning promises, which is for more than one QEMU version. Markus had an email about it last year. Daniel also suggested more version promises than what QAPI currently does. Anyway, that's my rationale for bool here. Cheers, Victor
signature.asc
Description: PGP signature
