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
