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

[Victor Toso]

Hi,

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

The current method signatures are:

    func MarshalEvent(e Event) ([]byte, error)
    func UnmarshalEvent(data []byte) (Event, error)

the suggestion is to change it to

    func EventToJSON(e Event) ([]byte, error)
    func EventFromJSON(data []byte) (Event, error)

or

    func JSONFromEvent(e Event) ([]byte, error)
    func EventFromJSON(data []byte) (Event, error)

? :)

I'm not picky with names so, what you find is best is okay for
me.

I'll be changing the function to avoid string manipulation in
favor of some anonymous structs.

Thanks you all for the review, really appreciate it!
Victor

signature.asc
Description: PGP signature