Re: [Xen-devel] [PATCH RFC 2/8] golang/xenlight: Add error constants and standard handling

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 18/01/17 19:56, Ronald Rojas wrote:
> Create error type Errorxl for throwing proper xenlight
> errors.
> 
> Update Ctx functions to throw Errorxl errors.
> 
> Signed-off-by: Ronald Rojas <ronladred@xxxxxxxxx>

Overall, looks good!  Thanks for this.  Just a few minor comments.

> ---
>  tools/golang/xenlight/xenlight.go | 77 
> +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/golang/xenlight/xenlight.go 
> b/tools/golang/xenlight/xenlight.go
> index 1f10e51..d58f8b8 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -32,6 +32,77 @@ import (
>  )
>  
>  /*
> + * Errors
> + */
> +type Errorxl int

Is there a reason not to make this just "Error"?  The callers will have
to use the namespace anyway (xenlight.Error).

> +
> +const (
> +     ErrorNonspecific                  = Errorxl(-C.ERROR_NONSPECIFIC)
> +     ErrorVersion                      = Errorxl(-C.ERROR_VERSION)
> +     ErrorFail                         = Errorxl(-C.ERROR_FAIL)
> +     ErrorNi                           = Errorxl(-C.ERROR_NI)
> +     ErrorNomem                        = Errorxl(-C.ERROR_NOMEM)
> +     ErrorInval                        = Errorxl(-C.ERROR_INVAL)
> +     ErrorBadfail                      = Errorxl(-C.ERROR_BADFAIL)
> +     ErrorGuestTimedout                = Errorxl(-C.ERROR_GUEST_TIMEDOUT)
> +     ErrorTimedout                     = Errorxl(-C.ERROR_TIMEDOUT)
> +     ErrorNoparavirt                   = Errorxl(-C.ERROR_NOPARAVIRT)
> +     ErrorNotReady                     = Errorxl(-C.ERROR_NOT_READY)
> +     ErrorOseventRegFail               = Errorxl(-C.ERROR_OSEVENT_REG_FAIL)
> +     ErrorBufferfull                   = Errorxl(-C.ERROR_BUFFERFULL)
> +     ErrorUnknownChild                 = Errorxl(-C.ERROR_UNKNOWN_CHILD)
> +     ErrorLockFail                     = Errorxl(-C.ERROR_LOCK_FAIL)
> +     ErrorJsonConfigEmpty              = Errorxl(-C.ERROR_JSON_CONFIG_EMPTY)
> +     ErrorDeviceExists                 = Errorxl(-C.ERROR_DEVICE_EXISTS)
> +     ErrorCheckpointDevopsDoesNotMatch = 
> Errorxl(-C.ERROR_CHECKPOINT_DEVOPS_DOES_NOT_MATCH)
> +     ErrorCheckpointDeviceNotSupported = 
> Errorxl(-C.ERROR_CHECKPOINT_DEVICE_NOT_SUPPORTED)
> +     ErrorVnumaConfigInvalid           = 
> Errorxl(-C.ERROR_VNUMA_CONFIG_INVALID)
> +     ErrorDomainNotfound               = Errorxl(-C.ERROR_DOMAIN_NOTFOUND)
> +     ErrorAborted                      = Errorxl(-C.ERROR_ABORTED)
> +     ErrorNotfound                     = Errorxl(-C.ERROR_NOTFOUND)
> +     ErrorDomainDestroyed              = Errorxl(-C.ERROR_DOMAIN_DESTROYED)
> +     ErrorFeatureRemoved               = Errorxl(-C.ERROR_FEATURE_REMOVED)
> +)

So here you define the constants as positive integers corresponding to
libxl's negative integers, so that you can use them for array indexes in
the string table below.

But when you return an error from libxl itself, you simply cast it to
Errorxl(), leaving it negative; this means that you can't actually
compare err with the constants here -- you have to negate it, which is a
bit strange.

I think probably negating the C libxl values for the golang constants,
as you have done here, makes sense.  But then you should negate the
values you get back from libxl as well, so that they match.

> +
> +var errors = [...]string{
> +     ErrorNonspecific:                  "Non-specific error",
> +     ErrorVersion:                      "Wrong version",
> +     ErrorFail:                         "Failed",
> +     ErrorNi:                           "Null",

"Not implemented"

> +     ErrorNomem:                        "No memory",
> +     ErrorInval:                        "Invalid",

probably "Invalid argument"

> +     ErrorBadfail:                      "Bad Fail",
> +     ErrorGuestTimedout:                "Guest timed out",
> +     ErrorTimedout:                     "Timed out",
> +     ErrorNoparavirt:                   "No Paravirtualization",
> +     ErrorNotReady:                     "Not ready",
> +     ErrorOseventRegFail:               "OS event failed",

"OS event registration failed"

> +     ErrorBufferfull:                   "Buffer full",
> +     ErrorUnknownChild:                 "Unknown child",
> +     ErrorLockFail:                     "Lock failed",
> +     ErrorJsonConfigEmpty:              "JSON config empyt",

empty

> +     ErrorDeviceExists:                 "Device exists",
> +     ErrorCheckpointDevopsDoesNotMatch: "Checkpoint devops does not match",
> +     ErrorCheckpointDeviceNotSupported: "Checkpoint device not supported",
> +     ErrorVnumaConfigInvalid:           "VNUMA config invalid",
> +     ErrorDomainNotfound:               "Domain not found",
> +     ErrorAborted:                      "Aborted",
> +     ErrorNotfound:                     "Not found",
> +     ErrorDomainDestroyed:              "Domain destroyed",
> +     ErrorFeatureRemoved:               "Feature removed",
> +}
> +
> +func (e Errorxl) Error() string {
> +     if 0 <= -int(e) && -int(e) < len(errors) {
> +             s := errors[-e]
> +             if s != "" {
> +                     return s
> +             }
> +     }
> +     return "errorxl " + strconv.Itoa(int(e))

You don't include the strconv package at this point.

I think since you're using fmt already, I'd just use fmt.Sprintf; and
I'd probably say, "libxl error: %d".  (Remembering to negate it again.)

> +}
> +
> +/*
>   * Types: Builtins
>   */
>  type Context struct {
> @@ -55,8 +126,7 @@ func (Ctx *Context) Open() (err error) {
>       ret := C.libxl_ctx_alloc(unsafe.Pointer(&Ctx.ctx), C.LIBXL_VERSION, 0, 
> nil)
>  
>       if ret != 0 {
> -             //FIXME: proper error
> -             err = createError("Allocating libxl context: ", ret)
> +             err = Errorxl(ret)
>       }
>       return
>  }
> @@ -66,8 +136,7 @@ func (Ctx *Context) Close() (err error) {
>       Ctx.ctx = nil
>  
>       if ret != 0 {
> -             //FIXME: proper error
> -             err = createError("Freeing libxl context: ", ret)
> +             err = Errorxl(ret)
>       }
>       return
>  }
> 

Hmm -- might be nice to have some golang-specific errors, like "Context
not open".  Not sure the best way to deal with that.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel