From 582b25986d53b33e701485e3218d647d12522298 Mon Sep 17 00:00:00 2001 From: Glyn Normington Date: Wed, 3 Sep 2014 16:03:41 -0700 Subject: [PATCH 1/2] Add rich errors to the API Add a rich Error type to the libcontainer package and use it in the API so that callers can: * Check for a specific error without depending on an error string, * Obtain the stack trace of the function or method which detected the error. The Error type provides a typed error code and a stack trace. The error code identifies the error and enables the caller to test for it without being sensitive to changes in the error text. The stack trace identifies the point at which the error was detected. The combination of error code and stack trace will enable errors to be diagnosed much more easily and with less guesswork than when raw string-based errors are used. The Error type conforms to the error interface and its Error method prints a short error message. The Detail method provides a verbose error message including the stack trace. Notes: 1. There is an unfortunate precedent in the Go standard library which uses variables to define errors. Checking for a specific error involves a string comparison and assumes the corresponding variable has not been updated. It is more robust and efficient to identify errors with integer-based types and associated constants, although errors should still include a string description for ease of use by humans. 2. It is not feasible to assign distinct types to Factory and Container error codes because common errors such as SystemError cannot be declared in two places and the names of the error codes then need to be decorated. This is less readable. Signed-off-by: Steve Powell --- container.go | 51 +++++++++++++++++++++++++++++---------------------- error.go | 38 ++++++++++++++++++++++++++++++++++++++ factory.go | 13 +++++++------ 3 files changed, 74 insertions(+), 28 deletions(-) create mode 100644 error.go diff --git a/container.go b/container.go index ace997b7..954ff320 100644 --- a/container.go +++ b/container.go @@ -14,58 +14,65 @@ type Container interface { // Returns the current run state of the container. // - // Errors: container no longer exists, - // system error. - RunState() (*RunState, error) + // Errors: + // ContainerDestroyed - Container no longer exists, + // SystemError - System error. + RunState() (*RunState, Error) // Returns the current config of the container. Config() *Config // Start a process inside the container. Returns the PID of the new process (in the caller process's namespace) and a channel that will return the exit status of the process whenever it dies. // - // Errors: container no longer exists, - // config is invalid, - // container is paused, - // system error. - Start(*ProcessConfig) (pid int, exitChan chan int, err error) + // Errors: + // ContainerDestroyed - Container no longer exists, + // ProcessConfigInvalid - config is invalid, + // ContainerPaused - Container is paused, + // SystemError - System error. + Start(config *ProcessConfig) (pid int, exitChan chan int, err Error) // Destroys the container after killing all running processes. // // Any event registrations are removed before the container is destroyed. // No error is returned if the container is already destroyed. // - // Errors: system error. - Destroy() error + // Errors: + // SystemError - System error. + Destroy() Error // Returns the PIDs inside this container. The PIDs are in the namespace of the calling process. // - // Errors: container no longer exists, - // system error. + // Errors: + // ContainerDestroyed - Container no longer exists, + // SystemError - System error. // // Some of the returned PIDs may no longer refer to processes in the Container, unless // the Container state is PAUSED in which case every PID in the slice is valid. - Processes() ([]int, error) + Processes() ([]int, Error) // Returns statistics for the container. // - // Errors: container no longer exists, - // system error. - Stats() (*ContainerStats, error) + // Errors: + // ContainerDestroyed - Container no longer exists, + // SystemError - System error. + Stats() (*ContainerStats, Error) // If the Container state is RUNNING or PAUSING, sets the Container state to PAUSING and pauses // the execution of any user processes. Asynchronously, when the container finished being paused the // state is changed to PAUSED. // If the Container state is PAUSED, do nothing. // - // Errors: container no longer exists, - // system error. - Pause() error + // Errors: + // ContainerDestroyed - Container no longer exists, + // SystemError - System error. + Pause() Error // If the Container state is PAUSED, resumes the execution of any user processes in the // Container before setting the Container state to RUNNING. // If the Container state is RUNNING, do nothing. // - // Errors: container no longer exists, - // system error. - Resume() error + // Errors: + // ContainerDestroyed - Container no longer exists, + // SystemError - System error. + Resume() Error } diff --git a/error.go b/error.go new file mode 100644 index 00000000..3b41e26f --- /dev/null +++ b/error.go @@ -0,0 +1,38 @@ +package libcontainer + +// API error code type. +type ErrorCode int + +// API error codes. +const ( + // Factory errors + IdInUse ErrorCode = iota + InvalidIdFormat + ConfigInvalid + // TODO: add Load errors + + // Container errors + ContainerDestroyed + ProcessConfigInvalid + ContainerPaused + + // Common errors + SystemError +) + +// API Error type. +type Error interface { + error + + // Returns the stack trace, if any, which identifies the + // point at which the error occurred. + Stack() []byte + + // Returns a verbose string including the error message + // and a representation of the stack trace suitable for + // printing. + Detail() string + + // Returns the error code for this error. + Code() ErrorCode +} diff --git a/factory.go b/factory.go index f4c31160..e37773b2 100644 --- a/factory.go +++ b/factory.go @@ -12,13 +12,13 @@ type Factory interface { // Returns the new container with a running process. // // Errors: - // id is already in use by a container - // id has incorrect format - // config is invalid - // System error + // IdInUse - id is already in use by a container + // InvalidIdFormat - id has incorrect format + // ConfigInvalid - config is invalid + // SystemError - System error // // On error, any partially created container parts are cleaned up (the operation is atomic). - Create(id string, config *Config) (Container, error) + Create(id string, config *Config) (Container, Error) // Load takes an ID for an existing container and reconstructs the container // from the state. @@ -27,5 +27,6 @@ type Factory interface { // Path does not exist // Container is stopped // System error - Load(id string) (Container, error) + // TODO: fix description + Load(id string) (Container, Error) } From 8453bee1ca766e3c36bdf0a06e58c2ff3e355256 Mon Sep 17 00:00:00 2001 From: Steve Powell Date: Fri, 5 Sep 2014 10:08:11 -0700 Subject: [PATCH 2/2] Unify the errors ProcessConfigInvalid and ConfigInvalid to avoid caller confusion. There is no check that these are being confused since they are both in the same type. Signed-off-by: Steve Powell --- container.go | 2 +- error.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/container.go b/container.go index 954ff320..307e8cbc 100644 --- a/container.go +++ b/container.go @@ -26,7 +26,7 @@ type Container interface { // // Errors: // ContainerDestroyed - Container no longer exists, - // ProcessConfigInvalid - config is invalid, + // ConfigInvalid - config is invalid, // ContainerPaused - Container is paused, // SystemError - System error. Start(config *ProcessConfig) (pid int, exitChan chan int, err Error) diff --git a/error.go b/error.go index 3b41e26f..5ff56d80 100644 --- a/error.go +++ b/error.go @@ -8,15 +8,14 @@ const ( // Factory errors IdInUse ErrorCode = iota InvalidIdFormat - ConfigInvalid // TODO: add Load errors // Container errors ContainerDestroyed - ProcessConfigInvalid ContainerPaused // Common errors + ConfigInvalid SystemError )