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

[Andrea Bolognani]

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.

-- 
Andrea Bolognani / Red Hat / Virtualization