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

[Daniel P . Berrangé]

On Wed, Jul 06, 2022 at 09:53:43AM -0500, Andrea Bolognani wrote:
> On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> > > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote:
> > > > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > > >     return s.EventTimestamp
> > > > }
> > >
> > > Does this even need a getter? The struct member is public, and Go
> > > code seems to favor direct member access.
> >
> > It satisfies the 'Event' interface, so you can fetch timestamp
> > without needing to know the specific event struct you're dealing
> > with.
> 
> Yeah but we're generating structs for all possible events ourselves
> and we don't really expect external implementations of this
> interface so I don't see what having this getter buys us over the
> guarantee, that we have by construction, that all events will have a
> public member with a specific name that contains the timestamp.

Code doesn't neccessarily want to deal with the per-event type
structs. It is credible to want to work with the abstract 'Event'
interface in places and being able to get the Timestamp from that,
without figuring out what specific event struct to cast it to first.

> > I don't think this kind of detail really needs to be exposed to
> > clients. Also parsing the same json doc twice just feels wrong.
> >
> > I think using the dummy union structs is the right approach, but
> > I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
> > make it clear this is different from a normal 'UnmarshalJSON'
> > method.
> 
> Fair point on avoiding parsing the JSON twice.
> 
> I still don't like the fact that we have to force clients to use a
> non-standard API, or that the API for events has to be different from
> that for unions. But Go won't let you add method implementations to
> an interface, so we're kinda stuck.

I think this specific case is out of scope for the "standard" JSON
APIs, and somewhere you'd expect APIs to do their own thing a layer
above, which is what we're doing here.

More importantly though what we're generating in terms of QAPI derived
APIs should be thought of as only the first step. Ultimately I would
not expect clients to be marshalling / unmarshalling these structs at
all. So from that POV I think the question of "non-standard" APIs is
largely irrelevant. 

Instead we would be providing a "QMPClient" type with APIs for sending/
receiving instances of the 'Command'/'Response' interfaces, along with
callback(s) that receives instances of the 'Event' interface.  Any JSON
marshalling is hidden behind the scenes as an internal imlp detail.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|