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

["Daniel P. Berrangé" <berrange@redhat.com>]

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:
> > This patch handles QAPI event types and generates data structures in
> > Go that handles it.
> >
> > We also define a Event interface and two helper functions MarshalEvent
> > and UnmarshalEvent.
> >
> > At the moment of this writing, this patch generates 51 structures (50
> > events)
> >
> > Example:
> >
> > qapi:
> >   | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> >   |   'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
> >
> > go:
> >   | type MemoryDeviceSizeChangeEvent struct {
> >   |         EventTimestamp Timestamp `json:"-"`
> >   |         Id             *string   `json:"id,omitempty"`
> >   |         Size           uint64    `json:"size"`
> >   |         QomPath        string    `json:"qom-path"`
> >   | }
> >
> > usage:
> >   | input := `{"event":"MEMORY_DEVICE_SIZE_CHANGE",` +
> >   |     `"timestamp":{"seconds":1588168529,"microseconds":201316},` +
> >   |     `"data":{"id":"vm0","size":1073741824,"qom-path":"/machine/unattached/device[2]"}}`
> >   | e, err := UnmarshalEvent([]byte(input)
> >   | if err != nil {
> >   |     panic(err)
> >   | }
> >   | if e.GetName() == `MEMORY_DEVICE_SIZE_CHANGE` {
> >   |     m := e.(*MemoryDeviceSizeChangeEvent)
> >   |     // m.QomPath == "/machine/unattached/device[2]"
> >   | }


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

> 
> > type Timestamp struct {
> >     Seconds      int64 `json:"seconds"`
> >     Microseconds int64 `json:"microseconds"`
> > }
> >
> > type Event interface {
> >     GetName() string
> >     GetTimestamp() Timestamp
> > }
> >


> > func UnmarshalEvent(data []byte) (Event, error) {
> >     base := struct {
> >         Name           string    `json:"event"`
> >         EventTimestamp Timestamp `json:"timestamp"`
> >     }{}
> >     if err := json.Unmarshal(data, &base); err != nil {
> >         return nil, errors.New(fmt.Sprintf("Failed to decode event: %s", string(data)))
> >     }
> >
> >     switch base.Name {
> >
> >     case "ACPI_DEVICE_OST":
> >         event := struct {
> >             Data AcpiDeviceOstEvent `json:"data"`
> >         }{}
> >
> >         if err := json.Unmarshal(data, &event); err != nil {
> >             return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s", string(data)))
> >         }
> >         event.Data.EventTimestamp = base.EventTimestamp
> >         return &event.Data, nil
> >
> >     // more cases here
> >     }
> >     return nil, errors.New("Failed to recognize event")
> > }
> 
> While I understand the need to have some way to figure out exactly
> what type of event we're looking at before being able to unmarshal
> the JSON data, I don't like how we force users to go through a
> non-standard API for it.
> 
> Here's how I think we should do it:
> 
>   func GetEventType(data []byte) (Event, error) {
>       type event struct {
>           Name string `json:"event"`
>       }
> 
>       tmp := event{}
>       if err := json.Unmarshal(data, &tmp); err != nil {
>           return nil, errors.New(fmt.Sprintf("Failed to decode event:
> %s", string(data)))
>       }
> 
>       switch tmp.Name {
>       case "ACPI_DEVICE_OST":
>           return &AcpiDeviceOstEvent{}, nil
> 
>       // more cases here
>       }
> 
>       return nil, errors.New("Failed to recognize event")
>   }
> 
>   func (s *AcpiDeviceOstEvent) UnmarshalJSON(data []byte) error {
>       type eventData struct {
>           Info ACPIOSTInfo `json:"info"`
>       }
>       type event struct {
>           Name           string    `json:"event"`
>           EventTimestamp Timestamp `json:"timestamp"`
>           Data           eventData `json:"data"`
>       }
> 
>       tmp := event{}
>       err := json.Unmarshal(data, &tmp)
>       if err != nil {
>           return err
>       }
>       if tmp.Name != "ACPI_DEVICE_OST" {
>           return errors.New("name")
>       }
> 
>       s.EventTimestamp = tmp.EventTimestamp
>       s.Info = tmp.Data.Info
>       return nil
>   }
> 
> This way, client code can look like
> 
>   in := `{"event":"ACPI_DEVICE_OST","timestamp":{"seconds":1265044230,"microseconds":450486},"data":{"info":{"device":"d1","slot":"0","slot-type":"DIMM","source":1,"status":0}}}`
> 
>   e, err := qapi.GetEventType([]byte(in))
>   if err != nil {
>       panic(err)
>   }
>   // e is of type AcpiDeviceOstEvent
> 
>   err = json.Unmarshal([]byte(in), &e)
>   if err != nil {
>       panic(err)
>   }
> 
> where only the first part (figuring out the type of the event) needs
> custom APIs, and the remaining code looks just like your average JSON
> handling.

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.

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