| Message ID | 20231016152704.221611-9-victortoso@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi-go: add generator for Golang interface | expand |
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.
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
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.
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py index bc6206797a..81b320d6dd 100644 --- a/scripts/qapi/golang.py +++ b/scripts/qapi/golang.py @@ -215,6 +215,54 @@ }} """ +TEMPLATE_EVENT = """ +type Timestamp struct {{ +\tSeconds int64 `json:"seconds"` +\tMicroseconds int64 `json:"microseconds"` +}} + +type Event interface {{ +\tGetName() string +\tGetTimestamp() Timestamp +}} + +func MarshalEvent(e Event) ([]byte, error) {{ +\tm := make(map[string]any) +\tm["event"] = e.GetName() +\tm["timestamp"] = e.GetTimestamp() +\tif bytes, err := json.Marshal(e); err != nil {{ +\t\treturn []byte{{}}, err +\t}} else if len(bytes) > 2 {{ +\t\tm["data"] = e +\t}} +\treturn json.Marshal(m) +}} + +func UnmarshalEvent(data []byte) (Event, error) {{ +\tbase := struct {{ +\t\tName string `json:"event"` +\t\tMessageTimestamp Timestamp `json:"timestamp"` +\t}}{{}} +\tif err := json.Unmarshal(data, &base); err != nil {{ +\t\treturn nil, fmt.Errorf("Failed to decode event: %s", string(data)) +\t}} + +\tswitch base.Name {{{cases} +\t}} +\treturn nil, errors.New("Failed to recognize event") +}} +""" + +TEMPLATE_EVENT_METHODS = """ +func (s *{type_name}) GetName() string {{ +\treturn "{name}" +}} + +func (s *{type_name}) GetTimestamp() Timestamp {{ +\treturn s.MessageTimestamp +}} +""" + def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None: vis = QAPISchemaGenGolangVisitor(prefix) @@ -238,7 +286,7 @@ def qapi_to_field_name_enum(name: str) -> str: return name.title().replace("-", "") -def qapi_to_go_type_name(name: str) -> str: +def qapi_to_go_type_name(name: str, meta: Optional[str] = None) -> str: if qapi_name_is_object(name): name = name[6:] @@ -253,6 +301,11 @@ def qapi_to_go_type_name(name: str) -> str: name += "".join(word.title() for word in words[1:]) + types = ["event"] + if meta in types: + name = name[:-3] if name.endswith("Arg") else name + name += meta.title().replace(" ", "") + return name @@ -608,6 +661,15 @@ def qapi_to_golang_struct( discriminator = None if not variants else variants.tag_member.name fields, with_nullable = recursive_base(self, base, discriminator) + if info.defn_meta == "event": + fields.insert( + 0, + { + "name": "MessageTimestamp", + "type": "Timestamp", + "tag": """`json:"-"`""", + }, + ) if members: for member in members: @@ -651,7 +713,7 @@ def qapi_to_golang_struct( fields.append(field) with_nullable = True if nullable else with_nullable - type_name = qapi_to_go_type_name(name) + type_name = qapi_to_go_type_name(name, info.defn_meta) content = generate_struct_type(type_name, fields, ident) if with_nullable: content += struct_with_nullable_generate_marshal( @@ -825,6 +887,28 @@ def generate_template_alternate( return content +def generate_template_event(events: dict[str, Tuple[str, str]]) -> str: + cases = "" + content = "" + for name in sorted(events): + case_type, gocode = events[name] + content += gocode + cases += f""" +\tcase "{name}": +\t\tevent := struct {{ +\t\t\tData {case_type} `json:"data"` +\t\t}}{{}} + +\t\tif err := json.Unmarshal(data, &event); err != nil {{ +\t\t\treturn nil, fmt.Errorf("Failed to unmarshal: %s", string(data)) +\t\t}} +\t\tevent.Data.MessageTimestamp = base.MessageTimestamp +\t\treturn &event.Data, nil +""" + content += TEMPLATE_EVENT.format(cases=cases) + return content + + def generate_content_from_dict(data: dict[str, str]) -> str: content = "" @@ -841,12 +925,14 @@ def __init__(self, _: str): types = ( "alternate", "enum", + "event", "helper", "struct", "union", ) self.target = dict.fromkeys(types, "") self.schema: QAPISchema + self.events: dict[str, Tuple[str, str]] = {} self.golang_package_name = "qapi" self.enums: dict[str, str] = {} self.alternates: dict[str, str] = {} @@ -901,6 +987,7 @@ def visit_end(self) -> None: self.target["alternate"] += generate_content_from_dict(self.alternates) self.target["struct"] += generate_content_from_dict(self.structs) self.target["union"] += generate_content_from_dict(self.unions) + self.target["event"] += generate_template_event(self.events) def visit_object_type( self, @@ -1027,7 +1114,36 @@ def visit_event( arg_type: Optional[QAPISchemaObjectType], boxed: bool, ) -> None: - pass + assert name == info.defn_name + assert name not in self.events + type_name = qapi_to_go_type_name(name, info.defn_meta) + + if isinstance(arg_type, QAPISchemaObjectType): + content = qapi_to_golang_struct( + self, + name, + arg_type.info, + arg_type.ifcond, + arg_type.features, + arg_type.base, + arg_type.members, + arg_type.variants, + ) + else: + args: List[dict[str:str]] = [] + args.append( + { + "name": "MessageTimestamp", + "type": "Timestamp", + "tag": """`json:"-"`""", + } + ) + content = generate_struct_type(type_name, args) + + content += TEMPLATE_EVENT_METHODS.format( + name=name, type_name=type_name + ) + self.events[name] = (type_name, content) def write(self, output_dir: str) -> None: for module_name, content in self.target.items():
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]" | } Signed-off-by: Victor Toso <victortoso@redhat.com> --- scripts/qapi/golang.py | 122 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 3 deletions(-)