Re: [PATCH v1 8/9] qapi: golang: Add CommandResult type to Go

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Sep 27, 2023 at 01:25:43PM +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.
> 
> Example:
> qapi:
>     | { 'command': 'query-sev', 'returns': 'SevInfo',
>     |   'if': 'TARGET_I386' }
> 
> go:
>     | type QuerySevCommandReturn struct {
>     |     CommandId 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.
>     |
>     | input := `{ "return": { "enabled": true, "api-major" : 0,` +
>     |                        `"api-minor" : 0, "build-id" : 0,` +
>     |                        `"policy" : 0, "state" : "running",` +
>     |                        `"handle" : 1 } } `
>     |
>     | ret := QuerySevCommandReturn{}
>     | err := json.Unmarshal([]byte(input), &ret)
>     | if ret.Error != nil {
>     |     // Handle command failure {"error": { ...}}
>     | } else if ret.Result != nil {
>     |     // ret.Result.Enable == true
>     | }
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  scripts/qapi/golang.py | 72 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> index 52a9124641..48ca0deab0 100644
> --- a/scripts/qapi/golang.py
> +++ b/scripts/qapi/golang.py
> @@ -40,6 +40,15 @@
>  '''
>  
>  TEMPLATE_HELPER = '''
> +type QapiError struct {

QAPIError as the name for this

> +    Class       string `json:"class"`
> +    Description string `json:"desc"`
> +}

> +
> +func (err *QapiError) Error() string {
> +    return fmt.Sprintf("%s: %s", err.Class, err.Description)
> +}

My gut feeling is that this should be just

    return err.Description

on the basis that long ago we pretty much decided that the
'Class' field was broadly a waste of time  except for a
couple of niche use cases. The error description is always
self contained and sufficient to diagnose problems, without
knowing the Class.

Keep the Class field in the struct though, as it could be
useful to check in certain cases


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|