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

[Victor Toso]

Hi,

On Thu, Nov 09, 2023 at 09:59:50AM -0800, Andrea Bolognani wrote:
> 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]"
>   }

I agree.

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

It might be useful for debugging too. I would rather log
e.GetName() than the string version of the type but if that's the
only reason we needed, I agree on removing for now.
 
> 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.

I agree. I'll change it. Perhaps we shouldn't use
(Un)MarshalEvent at this layer at all. Probably the same for
(Un)MarshalCommand.

Cheers,
Victor

signature.asc
Description: PGP signature