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

[Victor Toso]

Hi,

On Thu, Sep 28, 2023 at 04:03:12PM +0100, Daniel P. Berrangé wrote:
> 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

Agreed. I'll fix it.

> > +    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

I'll trust you on this. I'll change it.

Cheers,
Victor

signature.asc
Description: PGP signature