Re: [Xen-devel] Error reporting capabilities for libxc

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

On Fri, 2006-09-22 at 21:31 +0100, Daniel P. Berrange wrote:
> Currently APIs in the libxc library have a very limited means for reporting
> errors. They basically return -1 / NULL on error and set errno. Unfortunately
> since most of the errors which can occur in libxc are not related to system
> calls failing, the information available via errno is essentially useless.
> Indeed libxc simply sets errno to -EINVAL in most cases.
> 
> The irritating thing is that libxc *does* actually know exactly what went 
> wrong in all these cases & if you compile with -DDEBUG will report this
> information on STDERR. This is not much use for applications using libxc
> since they can hardly trap & parse STDERR to find out what went wrong!
> 
> As an illustration of why better error handling is needed, consider the
> frequently encountered problem of trying to create a non-PAE guest on a
> PAE hypervisor:
> 
>   # xm create demo
>   Using config file "demo".
>   Error: (22, 'Invalid argument')
> 
> This makes it essentially impossible to debug any errors with domain 
> creation, especially if -DDEBUG was not defined in your build of libxc

I agree; this is a serious problem that I suspect we've all run into at
least once.

> So, I've prototyped a simple error reporting mechanism for libxc. The idea
> is we first define an enum for the broad classes of errors which can occur.
> As proof of concept I've defined one generic error code, and a special one
> for invalid kernels
> 
>  typedef enum {
>     XC_ERROR_NONE = 0,
>     XC_INTERNAL_ERROR = 1,
>     XC_INVALID_KERNEL = 2,
>  } xc_error_code;
> 
> Next, I created an internal function  for setting the error code, and
> providing an associated descritive message
> 
>   void xc_set_error(int code, const char *fmt, ...);
> 
> And simply re-defined the existing ERROR & PERROR macros to use this 
> function passing the generic XC_INTERNAL_ERROR
> 
>  #define ERROR(_m, _a...) xc_set_error(XC_INTERNAL_ERROR, _m, ## _a)
> 
>  #define PERROR(_m, _a...) xc_set_error(XC_INTERNAL_ERROR, "%s (%d = %s)", 
> _m, errno, strerror(errno))
> 
> In the various places which deal with validating the guest kernel I
> changed calls to ERROR to call xc_set_error(XC_INVALID_KERNEL,...)
> instead. We can define further error codes over time to reduce use
> fo the genreic XC_INTERNAL_ERROR as desired.
> 
> In the public API defined a way to either fetch the last error, or
> register a callback for receving errors:
> 
>   int xc_get_last_error_code(void);
>   const char *xc_get_last_error_message(void);
>   void xc_clear_last_error(void);
>   const char *xc_error_code_to_desc(int code);
> 
>   typedef void (*xc_error_handler)(int code, const char *msg);
>   void xc_default_error_handler(int code, const char *msg);
>   xc_error_handler xc_set_error_handler(xc_error_handler handler);

Isn't the whole idea of "get_last_error" racy by design? I guess errno
has the same problem, but I'm not quite clear how it's solved there (a
magic array of errnos that is somehow sized to include all threads?)

> Any way there's some more cleanup to do & some error messages to improve,
> and switch more errors to use an explicit error code rather than the 
> generic  XC_INTERNAL_ERROR, but I thought I'd post the patch for review
> before going too much further.

Thanks for sending this out! It's really helpful to see the design
early.

-- 
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel