Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
On Thu, Nov 09, 2023 at 08:13:38PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:59:50AM -0800, Andrea Bolognani wrote:
> > 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.
I think the upside is too small considering the potential for abuse.
> > 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) {
> >
> > 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.
Yeah, what I wrote for events applies 1:1 to commands as well.
Up to you whether or not you want to keep around the convenience
functions. It might indeed be fine to drop them for now and consider
reintroducing them later if it turns out that it really helps making
client code less clunky.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
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, ); err != nil {
> return nil, err
> }
>
> switch tmp.Name {
> case "MEMORY_DEVICE_SIZE_CHANGE":
> return {}, 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
Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
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, ); err != nil {
return nil, err
}
switch tmp.Name {
case "MEMORY_DEVICE_SIZE_CHANGE":
return {}, 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
