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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6397 bytes --]

On Wed, 2017-01-18 at 14:56 -0500, 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@gmail.com>
> ---
>  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
> +
> +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)
> +)
> +
> +var errors = [...]string{
> +	ErrorNonspecific:                  "Non-specific error",
> +	ErrorVersion:                      "Wrong version",
> +	ErrorFail:                         "Failed",
> +	ErrorNi:                           "Null",
> +	ErrorNomem:                        "No memory",
> +	ErrorInval:                        "Invalid",
> +	ErrorBadfail:                      "Bad Fail",
> +	ErrorGuestTimedout:                "Guest timed out",
> +	ErrorTimedout:                     "Timed out",
> +	ErrorNoparavirt:                   "No Paravirtualization",
> +	ErrorNotReady:                     "Not ready",
> +	ErrorOseventRegFail:               "OS event failed",
> +	ErrorBufferfull:                   "Buffer full",
> +	ErrorUnknownChild:                 "Unknown child",
> +	ErrorLockFail:                     "Lock failed",
> +	ErrorJsonConfigEmpty:              "JSON config empyt",
> +	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))
> +}
> +
> +/*
>   * 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)
>
In general, it's not good practise to do something in patch n, and then
undo/redo it in patch n+1 (or in general n+m), within the same series.

Reason is, it makes things harder and longer to review (the same code
needs to be looked at, understood and, perhaps, commented twice!).

There are situations where that can't be avoided... but in this case,
it seems to me that the series can be reorganized in such a way that
errors/symbols are introduced before being used.

If that's not the case, because of Go specific characteristics that I'm
unaware of, then sorry for the noise (but then I'd say that these
specific characteristics could be at least quickly explained in a
changelog or a cover letter).

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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