|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 2/8] golang/xenlight: Add error constants and standard handling
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.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |