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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jan 19, 2017 at 04:02:46PM +0000, George Dunlap wrote:
> On 19/01/17 15:13, Ronald Rojas wrote:
> > 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@xxxxxxxxx>
> >>> ---
> >>>  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.
> 
> Which points out the problem with Dario's suggestion. :-)
> 
> It is indeed normal that you don't fix things from previous patches.
> But the "fix" here is from the very first patch: you can't introduce the
> error code before you introduce the directories and the makefile to
> build it.  The only way to avoid this "fix" would be to merge the two
> patches into one.
> 
> But that of course means that you're not separating out the important
> things from the first patch (the Makefile setup) with the important
> things from the second patch (the Error handling design).
> 
> I'd prefer it be left as it is; but it's Ian and Wei that have the final
> word on that.
> 

I'm fine with it as-is. I would rather avoid folding this patch into
the previous one.

Wei.

>  -George
> 

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