| Message ID | 20220617121932.249381-6-victortoso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: add generator for Golang interface | expand |
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.
On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote: > 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]" > > | } > > 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. It satisfies the 'Event' interface, so you can fetch timestamp without needing to know the specific event struct you're dealing with. > > > type Timestamp struct { > > Seconds int64 `json:"seconds"` > > Microseconds int64 `json:"microseconds"` > > } > > > > type Event interface { > > GetName() string > > GetTimestamp() Timestamp > > } > > > > 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. I don't think this kind of detail really needs to be exposed to clients. Also parsing the same json doc twice just feels wrong. I think using the dummy union structs is the right approach, but I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to make it clear this is different from a normal 'UnmarshalJSON' method. With regards, Daniel
On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote: > On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote: > > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote: > > > 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. > > It satisfies the 'Event' interface, so you can fetch timestamp > without needing to know the specific event struct you're dealing > with. Yeah but we're generating structs for all possible events ourselves and we don't really expect external implementations of this interface so I don't see what having this getter buys us over the guarantee, that we have by construction, that all events will have a public member with a specific name that contains the timestamp. > I don't think this kind of detail really needs to be exposed to > clients. Also parsing the same json doc twice just feels wrong. > > I think using the dummy union structs is the right approach, but > I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to > make it clear this is different from a normal 'UnmarshalJSON' > method. Fair point on avoiding parsing the JSON twice. I still don't like the fact that we have to force clients to use a non-standard API, or that the API for events has to be different from that for unions. But Go won't let you add method implementations to an interface, so we're kinda stuck. The only alternative I can think of would be to define Event as type Event struct { Shutdown *ShutdownEvent Reset *ResetEvent // and so on } which wouldn't be too bad from client code, as you'd only have to change from e, err := EventFromJSON(in) switch v := e.(type) { case ShutdownEvent: // do something with v case ResetEvent: // do something with v // and so on } to err := json.UnmarshalJSON(in, &e) if e.Shutdown != nil { // do something with e.Shutdown } else if e.Reset != nil { // do something with e.Reset } // and so on but that would mean each time we have to parse an event we'd end up allocating room for ~50 pointers and only actually using one, which is a massive waste.
On Wed, Jul 06, 2022 at 09:53:43AM -0500, Andrea Bolognani wrote: > On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote: > > On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote: > > > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote: > > > > 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. > > > > It satisfies the 'Event' interface, so you can fetch timestamp > > without needing to know the specific event struct you're dealing > > with. > > Yeah but we're generating structs for all possible events ourselves > and we don't really expect external implementations of this > interface so I don't see what having this getter buys us over the > guarantee, that we have by construction, that all events will have a > public member with a specific name that contains the timestamp. Code doesn't neccessarily want to deal with the per-event type structs. It is credible to want to work with the abstract 'Event' interface in places and being able to get the Timestamp from that, without figuring out what specific event struct to cast it to first. > > I don't think this kind of detail really needs to be exposed to > > clients. Also parsing the same json doc twice just feels wrong. > > > > I think using the dummy union structs is the right approach, but > > I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to > > make it clear this is different from a normal 'UnmarshalJSON' > > method. > > Fair point on avoiding parsing the JSON twice. > > I still don't like the fact that we have to force clients to use a > non-standard API, or that the API for events has to be different from > that for unions. But Go won't let you add method implementations to > an interface, so we're kinda stuck. I think this specific case is out of scope for the "standard" JSON APIs, and somewhere you'd expect APIs to do their own thing a layer above, which is what we're doing here. More importantly though what we're generating in terms of QAPI derived APIs should be thought of as only the first step. Ultimately I would not expect clients to be marshalling / unmarshalling these structs at all. So from that POV I think the question of "non-standard" APIs is largely irrelevant. Instead we would be providing a "QMPClient" type with APIs for sending/ receiving instances of the 'Command'/'Response' interfaces, along with callback(s) that receives instances of the 'Event' interface. Any JSON marshalling is hidden behind the scenes as an internal imlp detail. With regards, Daniel
On Wed, Jul 06, 2022 at 04:07:43PM +0100, Daniel P. Berrangé wrote: > On Wed, Jul 06, 2022 at 09:53:43AM -0500, Andrea Bolognani wrote: > > Yeah but we're generating structs for all possible events ourselves > > and we don't really expect external implementations of this > > interface so I don't see what having this getter buys us over the > > guarantee, that we have by construction, that all events will have a > > public member with a specific name that contains the timestamp. > > Code doesn't neccessarily want to deal with the per-event type > structs. It is credible to want to work with the abstract 'Event' > interface in places and being able to get the Timestamp from that, > without figuring out what specific event struct to cast it to first. Makes sense. > > I still don't like the fact that we have to force clients to use a > > non-standard API, or that the API for events has to be different from > > that for unions. But Go won't let you add method implementations to > > an interface, so we're kinda stuck. > > I think this specific case is out of scope for the "standard" JSON > APIs, and somewhere you'd expect APIs to do their own thing a layer > above, which is what we're doing here. > > More importantly though what we're generating in terms of QAPI derived > APIs should be thought of as only the first step. Ultimately I would > not expect clients to be marshalling / unmarshalling these structs at > all. So from that POV I think the question of "non-standard" APIs is > largely irrelevant. > > Instead we would be providing a "QMPClient" type with APIs for sending/ > receiving instances of the 'Command'/'Response' interfaces, along with > callback(s) that receives instances of the 'Event' interface. Any JSON > marshalling is hidden behind the scenes as an internal imlp detail. I will note that we want the marshalling/unmarshalling part and the wire transfer part to be somewhat loosely coupled, to allow for use cases that are not covered by the high-level client API, but overall I now agree with you that for most users this shouldn't ultimately matter :)
Hi, On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote: > 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? Yes, perhaps one that can be workaround in the next iteration. IIRC, I've added the type prefix to avoid the possibility of name clash with generated fields (like Info below). > > Info ACPIOSTInfo `json:"info"` > > } This happened with Command's Id that clashed with an Id for one or several other commands so I've changed it to "CommandId" and thought it would be wise to do the same for non generated fields. > > 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. Yeah, thanks. I'll change it. > > 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. As Daniel pointed out, just for the sake of working with a Event interface. > > 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 :) Yup. I agree that string manipulation was bad decision. I'll change it here too. > 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. I'll reply to this bit in the other email of this thread. > 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]" > } Yes, this seems much better/align with Go code. Many thanks for this! > 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 Cheers, Victor
Hi, On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote: > On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote: > > 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]" > > > | } > > > > > 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. > > It satisfies the 'Event' interface, so you can fetch timestamp > without needing to know the specific event struct you're dealing > with. > > > > > > type Timestamp struct { > > > Seconds int64 `json:"seconds"` > > > Microseconds int64 `json:"microseconds"` > > > } > > > > > > type Event interface { > > > GetName() string > > > GetTimestamp() Timestamp > > > } > > > > > > > > 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. > > I don't think this kind of detail really needs to be exposed to > clients. Also parsing the same json doc twice just feels wrong. > > I think using the dummy union structs is the right approach, but > I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to > make it clear this is different from a normal 'UnmarshalJSON' > method. The current method signatures are: func MarshalEvent(e Event) ([]byte, error) func UnmarshalEvent(data []byte) (Event, error) the suggestion is to change it to func EventToJSON(e Event) ([]byte, error) func EventFromJSON(data []byte) (Event, error) or func JSONFromEvent(e Event) ([]byte, error) func EventFromJSON(data []byte) (Event, error) ? :) I'm not picky with names so, what you find is best is okay for me. I'll be changing the function to avoid string manipulation in favor of some anonymous structs. Thanks you all for the review, really appreciate it! Victor
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py index 6c6a5cea97..b2e08cebdf 100644 --- a/scripts/qapi/golang.py +++ b/scripts/qapi/golang.py @@ -28,7 +28,66 @@ ) from .source import QAPISourceInfo +# Only variable is @unm_cases to handle +# all events's names and associated types. +TEMPLATE_EVENT = ''' +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 +}} + +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 {{ + {unm_cases} + }} + return nil, errors.New("Failed to recognize event") +}} +''' TEMPLATE_HELPER = ''' // Alias for go version lower than 1.18 type Any = interface{} @@ -53,10 +112,12 @@ class QAPISchemaGenGolangVisitor(QAPISchemaVisitor): def __init__(self, prefix: str): super().__init__() - self.target = {name: "" for name in ["alternate", "enum", "helper", "struct", + self.target = {name: "" for name in ["alternate", "enum", + "event", "helper", "struct", "union"]} self.objects_seen = {} self.schema = None + self.events = {} self.golang_package_name = "qapi" def visit_begin(self, schema): @@ -71,6 +132,24 @@ def visit_begin(self, schema): def visit_end(self): self.schema = None + unm_cases = "" + for name in sorted(self.events): + case_type = self.events[name] + unm_cases += f''' + case "{name}": + event := struct {{ + Data {case_type} `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 +''' + self.target["event"] += TEMPLATE_EVENT.format(unm_cases=unm_cases) + + def visit_object_type(self: QAPISchemaGenGolangVisitor, name: str, info: Optional[QAPISourceInfo], @@ -232,7 +311,37 @@ def visit_command(self, pass def visit_event(self, name, info, ifcond, features, arg_type, boxed): - pass + assert name == info.defn_name + type_name = qapi_to_go_type_name(name, info.defn_meta) + self.events[name] = type_name + + self_contained = True + if arg_type and arg_type.name.startswith("q_obj"): + self_contained = False + + content = "" + if self_contained: + content = generate_struct_type(type_name, '''EventTimestamp Timestamp `json:"-"`''') + else: + assert isinstance(arg_type, QAPISchemaObjectType) + content = qapi_to_golang_struct(name, + arg_type.info, + arg_type.ifcond, + arg_type.features, + arg_type.base, + arg_type.members, + arg_type.variants) + + methods = f''' +func (s *{type_name}) GetName() string {{ + return "{name}" +}} + +func (s *{type_name}) GetTimestamp() Timestamp {{ + return s.EventTimestamp +}} +''' + self.target["event"] += content + methods def write(self, output_dir: str) -> None: for module_name, content in self.target.items(): @@ -274,6 +383,8 @@ def qapi_to_golang_struct(name: str, type_name = qapi_to_go_type_name(name, info.defn_meta) fields = "" + if info.defn_meta == "event": + fields += '''\tEventTimestamp Timestamp `json:"-"`\n''' if base: base_fields = "" @@ -457,4 +568,9 @@ def qapi_to_go_type_name(name: str, meta: str) -> str: name = name.title() name += ''.join(word.title() for word in words[1:]) + + if meta in ["event"]: + name = name[:-3] if name.endswith("Arg") else name + name += meta.title().replace(" ", "") + return name
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]" | } Signed-off-by: Victor Toso <victortoso@redhat.com> --- scripts/qapi/golang.py | 120 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 2 deletions(-)