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
signature.asc
Description: PGP signature
