Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
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), )
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= error=errorDesc
The latter behavior seems more correct to me, based on how we deal
with unions and alternates.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
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
Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
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.
--
Andrea Bolognani / Red Hat / Virtualization
