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

[Andrea Bolognani]

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]"
>   | }

Generated code:

> type AcpiDeviceOstEvent struct {
>     EventTimestamp Timestamp   `json:"-"`

Any reason for naming this EventTimestamp as opposed to just
Timestamp?

>     Info           ACPIOSTInfo `json:"info"`
> }
>
> func (s *AcpiDeviceOstEvent) GetName() string {
>     return "ACPI_DEVICE_OST"
> }

Getters in Go don't usually start with "Get", so this could be just
Name(). And pointer receivers are usually only recommended when the
underlying object needs to be modified, which is not the case here.

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

> type Timestamp struct {
>     Seconds      int64 `json:"seconds"`
>     Microseconds int64 `json:"microseconds"`
> }
>
> type Event interface {
>     GetName() string
>     GetTimestamp() Timestamp
> }
>
> func MarshalEvent(e Event) ([]byte, error) {
>     baseStruct := struct {
>         Name           string    `json:"event"`
>         EventTimestamp Timestamp `json:"timestamp"`
>     }{
>         Name:           e.GetName(),
>         EventTimestamp: e.GetTimestamp(),
>     }
>     base, err := json.Marshal(baseStruct)
>     if err != nil {
>         return []byte{}, err
>     }
>
>     dataStruct := struct {
>         Payload Event `json:"data"`
>     }{
>         Payload: e,
>     }
>     data, err := json.Marshal(dataStruct)
>     if err != nil {
>         return []byte{}, err
>     }
>
>     if len(data) == len(`{"data":{}}`) {
>         return base, nil
>     }
>
>     // Combines Event's base and data in a single JSON object
>     result := fmt.Sprintf("%s,%s", base[:len(base)-1], data[1:])
>     return []byte(result), nil
> }

I have the same concerns about the string manipulation going on here
as I had for unions. Here's an alternative implementation, and this
time I've even tested it :)

  func (s AcpiDeviceOstEvent) MarshalJSON() ([]byte, error) {
      type eventData struct {
          Info ACPIOSTInfo `json:"info"`
      }
      type event struct {
          Name      string    `json:"event"`
          Timestamp Timestamp `json:"timestamp"`
          Data      eventData `json:"data"`
      }

      tmp := event{
          Name:      "ACPI_DEVICE_OST",
          Timestamp: s.EventTimestamp,
          Data:      eventData{
              Info: s.Info,
          },
      }
      return json.Marshal(tmp)
  }

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

Speaking of client code, in the commit message you have

> 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]"
> }

I don't think we should encourage the use of string comparison for
the purpose of going from a generic Event instance to a specific one:
a better way would be to use Go's type switch feature, such as

  switch m := e.(type) {
      case *MemoryDeviceSizeChangeEvent:
          // m.QomPath == "/machine/unattached/device[2]"
  }

In fact, I don't really see a reason to provide the Name() getter
outside of possibly diagnostics, and given the potential of it being
misused I would argue that it might be better not to have it.

-- 
Andrea Bolognani / Red Hat / Virtualization