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

[Ronald Rojas <ronladred@gmail.com>]

On Wed, Jan 18, 2017 at 11:16:31PM +0100, Dario Faggioli wrote:
> 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).

It's possible to add the errors as part of the first patch and then
add the context functions as the second patch as Go will at least 
let you compile the errors on it's own. I can swap the order of the
patchs in the next revision.

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

Thanks,
Ronald

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