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