Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go

[Victor Toso]

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