Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go

[Andrea Bolognani]

On Mon, Oct 16, 2023 at 05:27:01PM +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.
>
> Example:
> qapi:
>  | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
>  |   'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
>
> go:
>  | type MemoryDeviceSizeChangeEvent struct {
>  |         MessageTimestamp 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]"
>  | }

I don't think we should encourage people to perform string
comparisons, as it completely sidesteps Go's type system and is thus
error-prone. Safer version:

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

Now, I'm not sure I would go as far as suggesting that the GetName()
function should be completely removed, but maybe we can try leaving
it out from the initial version and see if people start screaming?

API-wise, I'm not a fan of the fact that we're forcing users to call
(Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
something like

  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, err
    }

    switch tmp.Name {
    case "MEMORY_DEVICE_SIZE_CHANGE":
            return &MemoryDeviceSizeChangeEvent{}, nil
    ...
    }

    return nil, fmt.Errorf("unrecognized event '%s'", tmp.Name)
  }

it becomes feasible to stick with standard functions. We can of
course keep the (Un)MarshalEvent functions around for convenience,
but I don't think they should be the only available API.

-- 
Andrea Bolognani / Red Hat / Virtualization