| Message ID | 20231016152704.221611-11-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:03PM +0200, Victor Toso wrote: > This patch adds a struct type in Go that will handle return values > for QAPI's command types. > > The return value of a Command is, encouraged to be, QAPI's complex > types or an Array of those. > > Every Command has a underlying CommandResult. The EmptyCommandReturn > is for those that don't expect any data e.g: `{ "return": {} }`. > > All CommandReturn types implement the CommandResult interface. I guess EmptyCommandReturn is something that you have scrapped in between revisions, because I see no reference to it in the current incarnation of the code. Same thing with CommandResult, unless that's just a typo for CommandReturn? > Example: > qapi: > | { 'command': 'query-sev', 'returns': 'SevInfo', > | 'if': 'TARGET_I386' } > > go: > | type QuerySevCommandReturn struct { > | MessageId string `json:"id,omitempty"` > | Result *SevInfo `json:"return"` > | Error *QapiError `json:"error,omitempty"` > | } > > usage: > | // One can use QuerySevCommandReturn directly or > | // command's interface GetReturnType() instead. I'm not convinced this function is particularly useful. I know that I've suggested something similar for events, but the usage scenarios are different. For events, you're going to have some loop listening for them and you can't know in advance what event you're going to receive at any given time. In contrast, a return value will be received as a direct consequence of running a command, and since the caller knows what command it called it's fair to assume that it also knows its return type. > + if ret_type: > + marshal_empty = "" > + ret_type_name = qapi_schema_type_to_go_type(ret_type.name) > + isptr = "*" if ret_type_name[0] not in "*[" else "" > + retargs.append( > + { > + "name": "Result", > + "type": f"{isptr}{ret_type_name}", > + "tag": """`json:"return"`""", > + } > + ) This produces type QueryAudiodevsCommandReturn struct { MessageId string `json:"id,omitempty"` Error *QAPIError `json:"error,omitempty"` Result []Audiodev `json:"return"` } when the return type is an array. Is that the correct behavior? I haven't thought too hard about it, but it seems odd so I though I'd bring it up.
Hi, On Thu, Nov 09, 2023 at 10:24:20AM -0800, Andrea Bolognani wrote: > On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote: > > This patch adds a struct type in Go that will handle return values > > for QAPI's command types. > > > > The return value of a Command is, encouraged to be, QAPI's complex > > types or an Array of those. > > > > Every Command has a underlying CommandResult. The EmptyCommandReturn > > is for those that don't expect any data e.g: `{ "return": {} }`. > > > > All CommandReturn types implement the CommandResult interface. > > I guess EmptyCommandReturn is something that you have scrapped > in between revisions, because I see no reference to it in the > current incarnation of the code. Same thing with CommandResult, > unless that's just a typo for CommandReturn? No typo. I did overlooked this commit log. We had EmptyCommandReturn type in the past, we agreed to have a specific type for each command even if they are empty (with basic/common fields only) > > > Example: > > qapi: > > | { 'command': 'query-sev', 'returns': 'SevInfo', > > | 'if': 'TARGET_I386' } > > > > go: > > | type QuerySevCommandReturn struct { > > | MessageId string `json:"id,omitempty"` > > | Result *SevInfo `json:"return"` > > | Error *QapiError `json:"error,omitempty"` > > | } > > > > usage: > > | // One can use QuerySevCommandReturn directly or > > | // command's interface GetReturnType() instead. > > I'm not convinced this function is particularly useful. I know > that I've suggested something similar for events, but the usage > scenarios are different. I think that I wanted to expose knowledge we had in the parser, not necessarily useful or needed indeed. At the very least, I agree that at this layer, we just want Command and ComandReturn types to be generated and properly (un)mashalled. One downside is for testing. If we have a list of commands, I can just iterate over them Unmarshal to a command interface variable, fetch the return type and do some comparisons to see if all is what we expected. See: https://gitlab.com/victortoso/qapi-go/-/blob/main/test/examples_test.go#L61 Not saying we should keep it for tests, but it is useful :) > For events, you're going to have some loop listening for them > and you can't know in advance what event you're going to > receive at any given time. > > In contrast, a return value will be received as a direct consequence > of running a command, and since the caller knows what command it > called it's fair to assume that it also knows its return type. > > > + if ret_type: > > + marshal_empty = "" > > + ret_type_name = qapi_schema_type_to_go_type(ret_type.name) > > + isptr = "*" if ret_type_name[0] not in "*[" else "" > > + retargs.append( > > + { > > + "name": "Result", > > + "type": f"{isptr}{ret_type_name}", > > + "tag": """`json:"return"`""", > > + } > > + ) > > This produces > > type QueryAudiodevsCommandReturn struct { > MessageId string `json:"id,omitempty"` > Error *QAPIError `json:"error,omitempty"` > Result []Audiodev `json:"return"` > } > > when the return type is an array. Is that the correct behavior? I > haven't thought too hard about it, but it seems odd so I though I'd > bring it up. Hm, the schema for it is ## # @query-audiodevs: # # Returns information about audiodev configuration # # Returns: array of @Audiodev # # Since: 8.0 ## { 'command': 'query-audiodevs', 'returns': ['Audiodev'] } So, I think it is correct. Would you expect it to be an object wrapping the array or I'm missing what you find odd. # -> { "execute": "query-rx-filter", "arguments": { "name": "vnet0" } } # <- { "return": [ # { # "promiscuous": true, # "name": "vnet0", # "main-mac": "52:54:00:12:34:56", # "unicast": "normal", # "vlan": "normal", # "vlan-table": [ # 4, # 0 # ], # "unicast-table": [ # ], # "multicast": "normal", # "multicast-overflow": false, # "unicast-overflow": false, # "multicast-table": [ # "01:00:5e:00:00:01", # "33:33:00:00:00:01", # "33:33:ff:12:34:56" # ], # "broadcast-allowed": false # } # ] # } ## { 'command': 'query-rx-filter', 'data': { '*name': 'str' }, 'returns': ['RxFilterInfo'] } Cheers, Victor
On Thu, Nov 09, 2023 at 08:31:01PM +0100, Victor Toso wrote: > On Thu, Nov 09, 2023 at 10:24:20AM -0800, Andrea Bolognani wrote: > > On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote: > > > Example: > > > qapi: > > > | { 'command': 'query-sev', 'returns': 'SevInfo', > > > | 'if': 'TARGET_I386' } > > > > > > go: > > > | type QuerySevCommandReturn struct { > > > | MessageId string `json:"id,omitempty"` > > > | Result *SevInfo `json:"return"` > > > | Error *QapiError `json:"error,omitempty"` > > > | } > > > > > > usage: > > > | // One can use QuerySevCommandReturn directly or > > > | // command's interface GetReturnType() instead. > > > > I'm not convinced this function is particularly useful. I know > > that I've suggested something similar for events, but the usage > > scenarios are different. > > I think that I wanted to expose knowledge we had in the parser, > not necessarily useful or needed indeed. At the very least, I > agree that at this layer, we just want Command and ComandReturn > types to be generated and properly (un)mashalled. > > One downside is for testing. > > If we have a list of commands, I can just iterate over them > Unmarshal to a command interface variable, fetch the return type > and do some comparisons to see if all is what we expected. See: > > https://gitlab.com/victortoso/qapi-go/-/blob/main/test/examples_test.go#L61 > > Not saying we should keep it for tests, but it is useful :) That code is quite dense and I'm not going to dig into it now :) Anyway, I don't have a problem with keeping this type-safe introspection feature around. Maybe call it GetCommandReturnType though. > > This produces > > > > type QueryAudiodevsCommandReturn struct { > > MessageId string `json:"id,omitempty"` > > Error *QAPIError `json:"error,omitempty"` > > Result []Audiodev `json:"return"` > > } > > > > when the return type is an array. Is that the correct behavior? I > > haven't thought too hard about it, but it seems odd so I though I'd > > bring it up. > > Hm, the schema for it is > > ## > # @query-audiodevs: > # > # Returns information about audiodev configuration > # > # Returns: array of @Audiodev > # > # Since: 8.0 > ## > { 'command': 'query-audiodevs', > 'returns': ['Audiodev'] } > > So, I think it is correct. Would you expect it to be an object > wrapping the array or I'm missing what you find odd. It works as expected if there is a result, but in the case of error: in := `{ "error": {"class": "errorClass", "desc": "errorDesc" }}` out := qapi.QueryAudiodevsCommandReturn{} err := json.Unmarshal([]byte(in), &out) if err != nil { panic(err) } fmt.Printf("result=%v error=%v\n", out.Result, out.Error) the output will be result=[] error=errorDesc Note how result is an empty array instead of a nil pointer. If we unmarshal the same JSON into QueryReplayCommandReturn instead, the output becomes result=<nil> error=errorDesc The latter behavior seems more correct to me, based on how we deal with unions and alternates.
diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py index 624bc2af4d..6471ddeb52 100644 --- a/scripts/qapi/golang.py +++ b/scripts/qapi/golang.py @@ -39,6 +39,15 @@ """ TEMPLATE_HELPER = """ +type QAPIError struct { +\tClass string `json:"class"` +\tDescription string `json:"desc"` +} + +func (err *QAPIError) Error() string { +\treturn err.Description +} + // Creates a decoder that errors on unknown Fields // Returns nil if successfully decoded @from payload to @into type // Returns error if failed to decode @from payload to @into type @@ -271,12 +280,17 @@ func (s *{type_name}) GetId() string {{ \treturn s.MessageId }} + +func (s *{type_name}) GetReturnType() CommandReturn {{ +\treturn &{cmd_ret_name}{{}} +}} """ TEMPLATE_COMMAND = """ type Command interface {{ \tGetId() string \tGetName() string +\tGetReturnType() CommandReturn }} func MarshalCommand(c Command) ([]byte, error) {{ @@ -308,6 +322,37 @@ }} """ +TEMPLATE_COMMAND_RETURN = """ +type CommandReturn interface { +\tGetId() string +\tGetCommandName() string +\tGetError() error +} +""" + +TEMPLATE_COMMAND_RETURN_METHODS = """ +func (r *{cmd_ret_name}) GetCommandName() string {{ +\treturn "{name}" +}} + +func (r *{cmd_ret_name}) GetId() string {{ +\treturn r.MessageId +}} + +func (r *{cmd_ret_name}) GetError() error {{ +\treturn r.Error +}}{marshal_empty} +""" + +TEMPLATE_COMMAND_RETURN_MARSHAL_EMPTY = """ +func (r {cmd_ret_name}) MarshalJSON() ([]byte, error) {{ +\tif r.Error != nil {{ +\t\ttype Alias {cmd_ret_name} +\t\treturn json.Marshal(Alias(r)) +\t}} +\treturn []byte(`{{"return":{{}}}}`), nil +}}""" + def gen_golang(schema: QAPISchema, output_dir: str, prefix: str) -> None: vis = QAPISchemaGenGolangVisitor(prefix) @@ -346,7 +391,7 @@ def qapi_to_go_type_name(name: str, meta: Optional[str] = None) -> str: name += "".join(word.title() for word in words[1:]) - types = ["event", "command"] + types = ["event", "command", "command return"] if meta in types: name = name[:-3] if name.endswith("Arg") else name name += meta.title().replace(" ", "") @@ -943,18 +988,19 @@ def generate_template_command(commands: dict[str, Tuple[str, str]]) -> str: case_type, gocode = commands[name] content += gocode cases += f""" -case "{name}": - command := struct {{ - Args {case_type} `json:"arguments"` - }}{{}} - - if err := json.Unmarshal(data, &command); err != nil {{ - return nil, fmt.Errorf("Failed to unmarshal: %s", string(data)) - }} - command.Args.MessageId = base.MessageId - return &command.Args, nil +\tcase "{name}": +\t\tcommand := struct {{ +\t\t\tArgs {case_type} `json:"arguments"` +\t\t}}{{}} + +\t\tif err := json.Unmarshal(data, &command); err != nil {{ +\t\t\treturn nil, fmt.Errorf("Failed to unmarshal: %s", string(data)) +\t\t}} +\t\tcommand.Args.MessageId = base.MessageId +\t\treturn &command.Args, nil """ content += TEMPLATE_COMMAND.format(cases=cases) + content += TEMPLATE_COMMAND_RETURN return content @@ -1182,6 +1228,34 @@ def visit_command( type_name = qapi_to_go_type_name(name, info.defn_meta) + cmd_ret_name = qapi_to_go_type_name(name, "command return") + marshal_empty = TEMPLATE_COMMAND_RETURN_MARSHAL_EMPTY.format( + cmd_ret_name=cmd_ret_name + ) + retargs: List[dict[str:str]] = [ + { + "name": "MessageId", + "type": "string", + "tag": """`json:"id,omitempty"`""", + }, + { + "name": "Error", + "type": "*QAPIError", + "tag": """`json:"error,omitempty"`""", + }, + ] + if ret_type: + marshal_empty = "" + ret_type_name = qapi_schema_type_to_go_type(ret_type.name) + isptr = "*" if ret_type_name[0] not in "*[" else "" + retargs.append( + { + "name": "Result", + "type": f"{isptr}{ret_type_name}", + "tag": """`json:"return"`""", + } + ) + content = "" if boxed or not arg_type or not qapi_name_is_object(arg_type.name): args: List[dict[str:str]] = [] @@ -1213,7 +1287,13 @@ def visit_command( ) content += TEMPLATE_COMMAND_METHODS.format( - name=name, type_name=type_name + name=name, type_name=type_name, cmd_ret_name=cmd_ret_name + ) + content += generate_struct_type(cmd_ret_name, retargs) + content += TEMPLATE_COMMAND_RETURN_METHODS.format( + name=name, + cmd_ret_name=cmd_ret_name, + marshal_empty=marshal_empty, ) self.commands[name] = (type_name, content)
This patch adds a struct type in Go that will handle return values for QAPI's command types. The return value of a Command is, encouraged to be, QAPI's complex types or an Array of those. Every Command has a underlying CommandResult. The EmptyCommandReturn is for those that don't expect any data e.g: `{ "return": {} }`. All CommandReturn types implement the CommandResult interface. Example: qapi: | { 'command': 'query-sev', 'returns': 'SevInfo', | 'if': 'TARGET_I386' } go: | type QuerySevCommandReturn struct { | MessageId string `json:"id,omitempty"` | Result *SevInfo `json:"return"` | Error *QapiError `json:"error,omitempty"` | } usage: | // One can use QuerySevCommandReturn directly or | // command's interface GetReturnType() instead. | | input := `{ "return": { "enabled": true, "api-major" : 0,` + | `"api-minor" : 0, "build-id" : 0,` + | `"policy" : 0, "state" : "running",` + | `"handle" : 1 } } ` | | ret := QuerySevCommandReturn{} | err := json.Unmarshal([]byte(input), &ret) | if ret.Error != nil { | // Handle command failure {"error": { ...}} | } else if ret.Result != nil { | // ret.Result.Enable == true | } Signed-off-by: Victor Toso <victortoso@redhat.com> --- scripts/qapi/golang.py | 104 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 92 insertions(+), 12 deletions(-)