Xen devel - Sep 2006 - Error reporting capabilities for libxc

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

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

In the python binding I replaced all use of PyErr_SetFromErrno with a helper
which fetches the info from xc_get_last_error_code/message.

Its important to note that in all this work I did not change the contract
of *any* public method in the C library for libxc. The return codes
are still the same, errno is still set in various places. If you compile
with -DDEBUG a error handler callback is set to print to STDERR. So any
existing users will continue to get exactly same behaviour as before. 
They do now have the ability to opt-in to getting full error details if
they so desire.

The python bindings are also unchanged with the exception that the error
thrown contains the xc_error_code & description instead of the errno
value. Since the latter was essentially useless I don''t think
that''s a
huge problem, but we could probably figure out a way to maintain compat
at the python level if absolutely required.

Any way, the upshot of all this work:

 - Try running a 32-bit PV kernel on 64-bit host:

  # xm create error
  Using config file "error".
  Error: [2, ''Kernel ELF architecture 3 does not match Xen architecture
62'']

 - Try running a non-PAE kernel on a PAE host:

  # xm create demo
  Using config file "demo".
  Error: [2, ''Non PAE-kernel on PAE host.'']

 - Try using libgtk as your kernel:

  # xm create error
  Using config file "error".
  Error: [2, ''Kernel ELF type 3 does not match Xen type 2'']

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.


  Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


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

Hi,

On Fri, 2006-09-22 at 21:31 +0100, Daniel P. Berrange wrote:
> 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.
...
> Any way, the upshot of all this work:
>   # xm create error
>   Using config file "error".
>   Error: [2, ''Kernel ELF architecture 3 does not match Xen
architecture 62'']
IMHO this is sorely needed.  Any comments from XenSource people?

--Stephen



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

On 26/9/06 12:45, "Stephen C. Tweedie" <sct@redhat.com> wrote:
>> 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.
> ...
> 
>> Any way, the upshot of all this work:
>>   # xm create error
>>   Using config file "error".
>>   Error: [2, ''Kernel ELF architecture 3 does not match Xen
architecture 62'']
> 
> IMHO this is sorely needed.  Any comments from XenSource people?
I''d agree something like this is necessary in the 3.0.4 timeframe.
I''ll let
one of the guys more acquainted with xend comment in detail. There''s
also
the question of how this will integrate with the proposed
''XenAPI''.

 -- Keir



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

> On 26/9/06 12:45, "Stephen C. Tweedie" <sct@redhat.com>
wrote:
> 
> >> 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.
> > ...
> > 
> >> Any way, the upshot of all this work:
> >>   # xm create error
> >>   Using config file "error".
> >>   Error: [2, ''Kernel ELF architecture 3 does not match
Xen
> >> architecture 62'']
> > 
> > IMHO this is sorely needed.  Any comments from XenSource people?
> 
> I''d agree something like this is necessary in the 3.0.4 
> timeframe. I''ll let one of the guys more acquainted with xend 
> comment in detail. There''s also the question of how this will 
> integrate with the proposed ''XenAPI''.
Yep, definitely needs to happen. However, I think we should at least
discuss alternative potentially less clunky implementation methods.
Perhaps we should use a thread local errno and error string variables?

Ian 


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

On Tue, Sep 26, 2006 at 01:28:37PM +0100, Keir Fraser
wrote:
> 
> 
> 
> On 26/9/06 12:45, "Stephen C. Tweedie" <sct@redhat.com>
wrote:
> 
> >> 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.
> > ...
> > 
> >> Any way, the upshot of all this work:
> >>   # xm create error
> >>   Using config file "error".
> >>   Error: [2, ''Kernel ELF architecture 3 does not match
Xen architecture 62'']
> > 
> > IMHO this is sorely needed.  Any comments from XenSource people?
> 
> I''d agree something like this is necessary in the 3.0.4 timeframe.
I''ll let
> one of the guys more acquainted with xend comment in detail.
There''s also
> the question of how this will integrate with the proposed
''XenAPI''.
I don''t anticipate any problems integrating with XenAPI -
we''ve previously
discussed the need to enumerate a set of error codes for XenAPI operations
and the need to also pass back descriptive string of the problem so I should
think its a good fit

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

On Tue, Sep 26, 2006 at 01:58:11PM +0100, Ian Pratt
wrote:
> > On 26/9/06 12:45, "Stephen C. Tweedie"
<sct@redhat.com> wrote:
> > 
> > >> 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.
> > > ...
> > > 
> > >> Any way, the upshot of all this work:
> > >>   # xm create error
> > >>   Using config file "error".
> > >>   Error: [2, ''Kernel ELF architecture 3 does not
match Xen
> > >> architecture 62'']
> > > 
> > > IMHO this is sorely needed.  Any comments from XenSource people?
> > 
> > I''d agree something like this is necessary in the 3.0.4 
> > timeframe. I''ll let one of the guys more acquainted with xend
> > comment in detail. There''s also the question of how this will
> > integrate with the proposed ''XenAPI''.
> 
> Yep, definitely needs to happen. However, I think we should at least
> discuss alternative potentially less clunky implementation methods.
> Perhaps we should use a thread local errno and error string variables?
Yes using thread locals instead of static vars is a good idea & should be
easy to implement behind the scenes. 

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

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@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Sep 26, 2006 at 01:40:01PM -0500, Hollis Blanchard
wrote:
> On Fri, 2006-09-22 at 21:31 +0100, Daniel P. Berrange wrote:
> > 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?)
Yeah, the libc errno  is kept in thread local storage, so we''ll have to
do the same thing here to be thread-safe. The async callback function is of
course always thread safe, but a little more inconvient for the caller to
use so I wanted to allow the direct access too. 

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

On Tue, Sep 26, 2006 at 01:40:01PM -0500, Hollis Blanchard wrote:
> 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?)
#define errno (*__errno_location())

I think recent glibc''s stash each thread''s errno in the
thread''s local
storage rather than using a global array, but haven''t checked.

Cheers,
Muli


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

On Tue, Sep 26, 2006 at 01:58:11PM +0100, Ian Pratt
wrote:
> > On 26/9/06 12:45, "Stephen C. Tweedie"
<sct@redhat.com> wrote:
> > 
> > >> 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.
> > > ...
> > > 
> > >> Any way, the upshot of all this work:
> > >>   # xm create error
> > >>   Using config file "error".
> > >>   Error: [2, ''Kernel ELF architecture 3 does not
match Xen
> > >> architecture 62'']
> > > 
> > > IMHO this is sorely needed.  Any comments from XenSource people?
> > 
> > I''d agree something like this is necessary in the 3.0.4 
> > timeframe. I''ll let one of the guys more acquainted with xend
> > comment in detail. There''s also the question of how this will
> > integrate with the proposed ''XenAPI''.
> 
> Yep, definitely needs to happen. However, I think we should at least
> discuss alternative potentially less clunky implementation methods.
> Perhaps we should use a thread local errno and error string variables?
Attached is an update to the original patch which annotates the static
variables with __thread so that they are setup per-thread. It also adds
more informative error reporting for bad kernels.

The only issue is that __thread is a GCC specific extension, so I''m not
sure this will work for the Solaris folks ?

  http://gcc.gnu.org/onlinedocs/gcc/Thread_002dLocal.html

The other option is to use the POSIX APIs  pthread_getspecific & 
pthread_setspecific. It''ll make the code alot more cumbersome, but
would certainly be portable. I''m certainly willing to do this though
if need be...

  Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


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

Daniel P. Berrange wrote:
> On Tue, Sep 26, 2006 at 01:58:11PM +0100, Ian Pratt wrote:
>>> On 26/9/06 12:45, "Stephen C. Tweedie"
<sct@redhat.com> wrote:
>>>
>>>>> 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.
>>>> ...
>>>>
>>>>> Any way, the upshot of all this work:
>>>>>   # xm create error
>>>>>   Using config file "error".
>>>>>   Error: [2, ''Kernel ELF architecture 3 does not
match Xen
>>>>> architecture 62'']
>>>> IMHO this is sorely needed.  Any comments from XenSource
people?
>>> I''d agree something like this is necessary in the 3.0.4 
>>> timeframe. I''ll let one of the guys more acquainted with
xend
>>> comment in detail. There''s also the question of how this
will
>>> integrate with the proposed ''XenAPI''.
>> Yep, definitely needs to happen. However, I think we should at least
>> discuss alternative potentially less clunky implementation methods.
>> Perhaps we should use a thread local errno and error string variables?
> 
> Attached is an update to the original patch which annotates the static
> variables with __thread so that they are setup per-thread. It also adds
> more informative error reporting for bad kernels.
What versions of GCC have you tested this with?

I''ve found in the past that __thread can have problems when using
shared
libraries (the exact instance was a Python binding).

Regards,

Anthony Liguori

> The only issue is that __thread is a GCC specific extension, so
I''m not
> sure this will work for the Solaris folks ?
> 
>   http://gcc.gnu.org/onlinedocs/gcc/Thread_002dLocal.html
> 
> The other option is to use the POSIX APIs  pthread_getspecific & 
> pthread_setspecific. It''ll make the code alot more cumbersome, but
> would certainly be portable. I''m certainly willing to do this
though
> if need be...
> 
>   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> 
> Regards,
> Dan.
> 
> 
> ------------------------------------------------------------------------
> 
> diff -ruN xen-3.0.3_0-src.orig/tools/libxc/xc_hvm_build.c
xen-3.0.3_0-src.new/tools/libxc/xc_hvm_build.c
> --- xen-3.0.3_0-src.orig/tools/libxc/xc_hvm_build.c	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/libxc/xc_hvm_build.c	2006-10-23
11:43:17.000000000 -0400
> @@ -460,7 +460,6 @@
>                       ctxt, domctl.u.getdomaininfo.shared_info_frame,
>                       vcpus, pae, acpi, apic, store_evtchn, store_mfn) <
0)
>      {
> -        ERROR("Error constructing guest OS");
>          goto error_out;
>      }
>  
> @@ -529,26 +528,30 @@
>  
>      if ( !IS_ELF(*ehdr) )
>      {
> -        ERROR("Kernel image does not have an ELF header.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Kernel image does not have an ELF
header.");
>          return -EINVAL;
>      }
>  
>      if ( (ehdr->e_phoff + (ehdr->e_phnum * ehdr->e_phentsize))
> elfsize )
>      {
> -        ERROR("ELF program headers extend beyond end of
image.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF program headers extend beyond end of
image.");
>          return -EINVAL;
>      }
>  
>      if ( (ehdr->e_shoff + (ehdr->e_shnum * ehdr->e_shentsize))
> elfsize )
>      {
> -        ERROR("ELF section headers extend beyond end of
image.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF section headers extend beyond end of
image.");
>          return -EINVAL;
>      }
>  
>      /* Find the section-header strings table. */
>      if ( ehdr->e_shstrndx == SHN_UNDEF )
>      {
> -        ERROR("ELF image has no section-header strings table
(shstrtab).");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF image has no section-header strings table
(shstrtab).");
>          return -EINVAL;
>      }
>      shdr = (Elf32_Shdr *)(elfbase + ehdr->e_shoff +
> @@ -570,7 +573,8 @@
>           (ehdr->e_entry < kernstart) ||
>           (ehdr->e_entry > kernend) )
>      {
> -        ERROR("Malformed ELF image.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Malformed ELF image.");
>          return -EINVAL;
>      }
>  
> diff -ruN xen-3.0.3_0-src.orig/tools/libxc/xc_linux_build.c
xen-3.0.3_0-src.new/tools/libxc/xc_linux_build.c
> --- xen-3.0.3_0-src.orig/tools/libxc/xc_linux_build.c	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/libxc/xc_linux_build.c	2006-10-23
13:30:13.000000000 -0400
> @@ -121,7 +121,7 @@
>      if ( probe_elf(image, image_size, load_funcs) &&
>           probe_bin(image, image_size, load_funcs) )
>      {
> -        ERROR( "Unrecognized image format" );
> +        xc_set_error(XC_INVALID_KERNEL, "Not a valid ELF or raw
kernel image");
>          return -EINVAL;
>      }
>  
> @@ -611,17 +611,20 @@
>      xen_capabilities_info_t xen_caps = "";
>  
>      if (xc_version(xc_handle, XENVER_capabilities, &xen_caps) != 0) {
> -        ERROR("Cannot determine host capabilities.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Cannot determine host capabilities.");
>          return 0;
>      }
>  
>      if (strstr(xen_caps, "xen-3.0-x86_32p")) {
>          if (dsi->pae_kernel == PAEKERN_no) {
> -            ERROR("Non PAE-kernel on PAE host.");
> +            xc_set_error(XC_INVALID_KERNEL,
> +                         "Non PAE-kernel on PAE host.");
>              return 0;
>          }
>      } else if (dsi->pae_kernel != PAEKERN_no) {
> -        ERROR("PAE-kernel on non-PAE host.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "PAE-kernel on non-PAE host.");
>          return 0;
>      }
>  
> @@ -1167,7 +1170,6 @@
>                       console_evtchn, console_mfn,
>                       features_bitmap) < 0 )
>      {
> -        ERROR("Error constructing guest OS");
>          goto error_out;
>      }
>  
> diff -ruN xen-3.0.3_0-src.orig/tools/libxc/xc_load_elf.c
xen-3.0.3_0-src.new/tools/libxc/xc_load_elf.c
> --- xen-3.0.3_0-src.orig/tools/libxc/xc_load_elf.c	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/libxc/xc_load_elf.c	2006-10-23
13:36:06.000000000 -0400
> @@ -29,20 +29,46 @@
>   */
>  #if defined(__ia64__)
>  #define ELFCLASS   ELFCLASS64
> +#define ELFCLASS_DESC "64-bit"
> +
>  #define ELFDATA    ELFDATA2LSB
> +#define ELFDATA_DESC "Little-Endian"
> +
>  #define ELFMACHINE EM_IA_64
> +#define ELFMACHINE_DESC "ia64"
> +
> +
>  #elif defined(__i386__)
>  #define ELFCLASS   ELFCLASS32
> +#define ELFCLASS_DESC "32-bit"
> +
>  #define ELFDATA    ELFDATA2LSB
> +#define ELFDATA_DESC "Little-Endian"
> +
>  #define ELFMACHINE EM_386
> +#define ELFMACHINE_DESC "i386"
> +
> +
>  #elif defined(__x86_64__)
>  #define ELFCLASS   ELFCLASS64
> +#define ELFCLASS_DESC "64-bit"
> +
>  #define ELFDATA    ELFDATA2LSB
> +#define ELFDATA_DESC "Little-Endian"
> +
>  #define ELFMACHINE EM_X86_64
> +#define ELFMACHINE_DESC "x86_64"
> +
> +
>  #elif defined(__powerpc__)
>  #define ELFCLASS   ELFCLASS64
> +#define ELFCLASS_DESC "64-bit"
> +
>  #define ELFDATA    ELFDATA2MSB
> +#define ELFDATA_DESC "Big-Endian"
> +
>  #define ELFMACHINE EM_PPC64
> +#define ELFMACHINE_DESC "ppc64"
>  #endif
>  
>  int probe_elf(const char *image,
> @@ -231,7 +257,8 @@
>          *defined = 1;
>          return *(uint64_t*)ELFNOTE_DESC(note);
>      default:
> -        ERROR("elfnotes: unknown data size %#x for numeric type note
%#x\n",
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "elfnotes: unknown data size %#x for numeric
type note %#x\n",
>                note->descsz, type);
>          return 0;
>      }
> @@ -250,35 +277,59 @@
>  
>      if ( !IS_ELF(*ehdr) )
>      {
> -        ERROR("Kernel image does not have an ELF header.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Kernel image does not have an ELF
header.");
>          return -EINVAL;
>      }
>  
> -    if ( (ehdr->e_ident[EI_CLASS] != ELFCLASS) ||
> -         (ehdr->e_machine != ELFMACHINE) ||
> -         (ehdr->e_ident[EI_DATA] != ELFDATA) ||
> -         (ehdr->e_type != ET_EXEC) )
> +    if (ehdr->e_machine != ELFMACHINE)
> +    {
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Kernel ELF architecture ''%d''
does not match Xen architecture ''%d'' (%s)",
> +                     ehdr->e_machine, ELFMACHINE, ELFMACHINE_DESC);
> +        return -EINVAL;
> +    }
> +    if (ehdr->e_ident[EI_CLASS] != ELFCLASS)
> +    {
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Kernel ELF wordsize ''%d'' does
not match Xen wordsize ''%d'' (%s)",
> +                     ehdr->e_ident[EI_CLASS], ELFCLASS, ELFCLASS_DESC);
> +        return -EINVAL;
> +    }
> +    if (ehdr->e_ident[EI_DATA] != ELFDATA)
> +    {
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Kernel ELF endianness ''%d''
does not match Xen endianness ''%d'' (%s)",
> +                     ehdr->e_ident[EI_DATA], ELFDATA, ELFDATA_DESC);
> +        return -EINVAL;
> +    }
> +    if (ehdr->e_type != ET_EXEC)
>      {
> -        ERROR("Kernel not a Xen-compatible Elf image.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Kernel ELF type ''%d'' does not
match Xen type ''%d''",
> +                     ehdr->e_type, ET_EXEC);
>          return -EINVAL;
>      }
>  
>      if ( (ehdr->e_phoff + (ehdr->e_phnum*ehdr->e_phentsize)) >
image_len )
>      {
> -        ERROR("ELF program headers extend beyond end of
image.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF program headers extend beyond end of
image.");
>          return -EINVAL;
>      }
>  
>      if ( (ehdr->e_shoff + (ehdr->e_shnum*ehdr->e_shentsize)) >
image_len )
>      {
> -        ERROR("ELF section headers extend beyond end of
image.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF section headers extend beyond end of
image.");
>          return -EINVAL;
>      }
>  
>      /* Find the section-header strings table. */
>      if ( ehdr->e_shstrndx == SHN_UNDEF )
>      {
> -        ERROR("ELF image has no section-header strings table
(shstrtab).");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF image has no section-header strings table
(shstrtab).");
>          return -EINVAL;
>      }
>      shdr = (Elf_Shdr *)(image + ehdr->e_shoff +
> @@ -325,22 +376,25 @@
>          if ( ( loader == NULL || strncmp(loader, "generic", 7) )
&&
>               ( guest_os == NULL || strncmp(guest_os, "linux", 5)
) )
>          {
> -            ERROR("Will only load images built for the generic loader
"
> -                  "or Linux images");
> +            xc_set_error(XC_INVALID_KERNEL,
> +                         "Will only load images built for the generic
loader "
> +                         "or Linux images");
>              return -EINVAL;
>          }
>  
>          if ( xen_version == NULL || strncmp(xen_version,
"xen-3.0", 7) )
>          {
> -            ERROR("Will only load images built for Xen v3.0");
> +            xc_set_error(XC_INVALID_KERNEL,
> +                         "Will only load images built for Xen
v3.0");
>              return -EINVAL;
>          }
>      }
>      else
>      {
>  #if defined(__x86_64__) || defined(__i386__)
> -        ERROR("Not a Xen-ELF image: "
> -              "No ELF notes or ''__xen_guest''
section found.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Not a Xen-ELF image: "
> +                     "No ELF notes or ''__xen_guest''
section found.");
>          return -EINVAL;
>  #endif
>      }
> @@ -396,8 +450,9 @@
>  
>      if ( elf_pa_off_defined && !virt_base_defined )
>      {
> -        ERROR("Neither ELF_PADDR_OFFSET nor VIRT_BASE found in ELF
"
> -              " notes or __xen_guest section.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "Neither ELF_PADDR_OFFSET nor VIRT_BASE found in
ELF "
> +                     " notes or __xen_guest section.");
>          return -EINVAL;
>      }
>  
> @@ -409,7 +464,8 @@
>          vaddr = phdr->p_paddr - dsi->elf_paddr_offset +
dsi->v_start;
>          if ( (vaddr + phdr->p_memsz) < vaddr )
>          {
> -            ERROR("ELF program header %d is too large.", h);
> +            xc_set_error(XC_INVALID_KERNEL,
> +                         "ELF program header %d is too large.",
h);
>              return -EINVAL;
>          }
>  
> @@ -431,7 +487,8 @@
>           (dsi->v_kernentry > kernend) ||
>           (dsi->v_start > kernstart) )
>      {
> -        ERROR("ELF start or entries are out of bounds.");
> +        xc_set_error(XC_INVALID_KERNEL,
> +                     "ELF start or entries are out of bounds.");
>          return -EINVAL;
>      }
>  
> diff -ruN xen-3.0.3_0-src.orig/tools/libxc/xc_private.c
xen-3.0.3_0-src.new/tools/libxc/xc_private.c
> --- xen-3.0.3_0-src.orig/tools/libxc/xc_private.c	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/libxc/xc_private.c	2006-10-23
13:47:54.000000000 -0400
> @@ -6,6 +6,85 @@
>  
>  #include <inttypes.h>
>  #include "xc_private.h"
> +#include <stdarg.h>
> +
> +static __thread int last_error_code = XC_ERROR_NONE;
> +static __thread char *last_error_message = NULL;
> +#if DEBUG
> +static xc_error_handler error_handler = xc_default_error_handler;
> +#else
> +static xc_error_handler error_handler = NULL;
> +#endif
> +
> +void xc_default_error_handler(int code, const char *msg) {
> +    const char *desc = xc_error_code_to_desc(code);
> +    fprintf(stderr, "ERROR %s: %s\n", desc, msg);
> +}
> +
> +int xc_get_last_error_code(void) {
> +    return last_error_code;
> +}
> +
> +const char *xc_get_last_error_message(void) {
> +    return last_error_message;
> +}
> +
> +void xc_clear_last_error(void) {
> +    last_error_code = XC_ERROR_NONE;
> +    if (last_error_message) {
> +        free(last_error_message);
> +        last_error_message = NULL;
> +    }
> +}
> +
> +const char *xc_error_code_to_desc(int code) {
> +    /* Sync to members of xc_error_code enumeration in xenctrl.h */
> +    switch (code) {
> +    case XC_ERROR_NONE: return "No error details";
> +    case XC_INTERNAL_ERROR: return "Internal error";
> +    case XC_INVALID_KERNEL: return "Invalid kernel";
> +    default:
> +        return "Unknown error code";
> +    }
> +}
> +
> +xc_error_handler xc_set_error_handler(xc_error_handler handler) {
> +    xc_error_handler old = error_handler;
> +    error_handler = handler;
> +    return old;
> +}
> +
> +
> +static void _xc_set_error(int code, const char *msg) {
> +    xc_clear_last_error();
> +    last_error_code = code;
> +    /* NB, we delibrately ignore the OOM condition when
> +       strdup()''ing here - there''s not really anything
> +       we can - particularly if the error we''re reporting
> +       was an OOM in the first place. The caller can always
> +       find the latter via errno */
> +    last_error_message = strdup(msg);
> +}
> +
> +#define XC_MAX_ERROR_MSG 4096
> +void xc_set_error(int code, const char *fmt, ...) {
> +    int saved_errno = errno;
> +    char msg[XC_MAX_ERROR_MSG];
> +    va_list args;
> +
> +    va_start(args, fmt);
> +    vsnprintf(msg, XC_MAX_ERROR_MSG-1, fmt, args);
> +    msg[XC_MAX_ERROR_MSG-1] = ''\0'';
> +    va_end(args);
> +
> +    _xc_set_error(code, msg);
> +
> +    errno = saved_errno;
> +
> +    if (error_handler)
> +        error_handler(code, msg);
> +}
> +
>  
>  /* NB: arr must be mlock''ed */
>  int xc_get_pfn_type_batch(int xc_handle,
> diff -ruN xen-3.0.3_0-src.orig/tools/libxc/xc_private.h
xen-3.0.3_0-src.new/tools/libxc/xc_private.h
> --- xen-3.0.3_0-src.orig/tools/libxc/xc_private.h	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/libxc/xc_private.h	2006-10-23
11:43:17.000000000 -0400
> @@ -56,25 +56,13 @@
>  #define PPRINTF(_f, _a...)
>  #endif
>  
> -#define ERR(_f, _a...) do {                     \
> -    DPRINTF(_f ": %d\n" , ## _a, errno);        \
> -    fflush(stderr); }                           \
> -while (0)
> -
> -#define ERROR(_m, _a...)                        \
> -do {                                            \
> -    int __saved_errno = errno;                  \
> -    DPRINTF("ERROR: " _m "\n" , ## _a );        \
> -    errno = __saved_errno;                      \
> -} while (0)
> -
> -#define PERROR(_m, _a...)                               \
> -do {                                                    \
> -    int __saved_errno = errno;                          \
> -    DPRINTF("ERROR: " _m " (%d = %s)\n" , ## _a ,     
\
> -            __saved_errno, strerror(__saved_errno));    \
> -    errno = __saved_errno;                              \
> -} while (0)
> +void xc_set_error(int code, const char *fmt, ...);
> +
> +#define ERR(_f, _a...) ERROR(_f, ## _a)
> +
> +#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))
>  
>  static inline void safe_munlock(const void *addr, size_t len)
>  {
> diff -ruN xen-3.0.3_0-src.orig/tools/libxc/xenctrl.h
xen-3.0.3_0-src.new/tools/libxc/xenctrl.h
> --- xen-3.0.3_0-src.orig/tools/libxc/xenctrl.h	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/libxc/xenctrl.h	2006-10-23 11:46:31.000000000
-0400
> @@ -656,4 +656,43 @@
>   */
>  int xc_evtchn_unmask(int xce_handle, evtchn_port_t port);
>  
> +
> +typedef enum {
> +  XC_ERROR_NONE = 0,
> +  XC_INTERNAL_ERROR = 1,
> +  XC_INVALID_KERNEL = 2,
> +} xc_error_code;
> +
> +/*
> + * Return the code of the last error to occurr
> + */
> +int xc_get_last_error_code(void);
> +
> +/*
> + * Return the message associated with the last error to occur
> + */
> +const char *xc_get_last_error_message(void);
> +
> +/*
> + * Clear the last error
> + */
> +void xc_clear_last_error(void);
> +
> +typedef void (*xc_error_handler)(int code, const char *msg);
> +
> +/*
> + * The default error handler which prints to stderr
> + */
> +void xc_default_error_handler(int code, const char *msg);
> +
> +/*
> + * Convert an error code into a text description
> + */
> +const char *xc_error_code_to_desc(int code);
> +
> +/*
> + * Registers a callback to handle errors
> + */
> +xc_error_handler xc_set_error_handler(xc_error_handler handler);
> +
>  #endif
> diff -ruN xen-3.0.3_0-src.orig/tools/python/xen/lowlevel/xc/xc.c
xen-3.0.3_0-src.new/tools/python/xen/lowlevel/xc/xc.c
> --- xen-3.0.3_0-src.orig/tools/python/xen/lowlevel/xc/xc.c	2006-10-15
08:22:03.000000000 -0400
> +++ xen-3.0.3_0-src.new/tools/python/xen/lowlevel/xc/xc.c	2006-10-23
11:50:00.000000000 -0400
> @@ -37,6 +37,24 @@
>  static PyObject *dom_op(XcObject *self, PyObject *args,
>                          int (*fn)(int, uint32_t));
>  
> +static PyObject *pyxc_error_to_exception(void)
> +{
> +    PyObject *err;
> +    int code = xc_get_last_error_code();
> +    const char *desc = xc_error_code_to_desc(code);
> +    const char *msg = xc_get_last_error_message();
> +
> +    if (msg)
> +	err = Py_BuildValue("[iss]", code, desc, msg);
> +    else
> +	err = Py_BuildValue("[is]", code, desc);
> +
> +    xc_clear_last_error();
> +
> +    PyErr_SetObject(xc_error, err);
> +
> +    return NULL;
> +}
>  
>  static PyObject *pyxc_domain_dumpcore(XcObject *self, PyObject *args)
>  {
> @@ -50,7 +68,7 @@
>          return NULL;
>  
>      if (xc_domain_dumpcore(self->xc_handle, dom, corefile) != 0)
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -95,12 +113,11 @@
>      }
>  
>      if ( (ret = xc_domain_create(self->xc_handle, ssidref, handle,
&dom)) < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return PyInt_FromLong(dom);
>  
>  out_exception:
> -    errno = EINVAL;
>      PyErr_SetFromErrno(xc_error);
>      return NULL;
>  }
> @@ -113,7 +130,7 @@
>        return NULL;
>  
>      if (xc_domain_max_vcpus(self->xc_handle, dom, max) != 0)
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -158,7 +175,7 @@
>      }
>    
>      if ( xc_vcpu_setaffinity(self->xc_handle, dom, vcpu, cpumap) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -178,7 +195,7 @@
>          return NULL;
>  
>      if ( xc_domain_setcpuweight(self->xc_handle, dom, cpuweight) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -209,13 +226,12 @@
>      }
>  
>      if (xc_domain_sethandle(self->xc_handle, dom, handle) < 0)
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
>  
>  out_exception:
> -    errno = EINVAL;
>      PyErr_SetFromErrno(xc_error);
>      return NULL;
>  }
> @@ -245,7 +261,7 @@
>      if (nr_doms < 0)
>      {
>          free(info);
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      }
>  
>      list = PyList_New(nr_doms);
> @@ -299,10 +315,10 @@
>  
>      rc = xc_vcpu_getinfo(self->xc_handle, dom, vcpu, &info);
>      if ( rc < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      rc = xc_vcpu_getaffinity(self->xc_handle, dom, vcpu, &cpumap);
>      if ( rc < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      info_dict = Py_BuildValue("{s:i,s:i,s:i,s:L,s:i}",
>                                "online",   info.online,
> @@ -352,9 +368,7 @@
>                          ramdisk, cmdline, features, flags,
>                          store_evtchn, &store_mfn,
>                          console_evtchn, &console_mfn) != 0 ) {
> -        if (!errno)
> -             errno = EINVAL;
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      }
>      return Py_BuildValue("{s:i,s:i}", 
>                           "store_mfn", store_mfn,
> @@ -385,7 +399,7 @@
>  
>      if ( xc_hvm_build(self->xc_handle, dom, memsize, image,
>                        vcpus, pae, acpi, apic, store_evtchn,
&store_mfn) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return Py_BuildValue("{s:i}", "store_mfn",
store_mfn);
>  }
> @@ -404,7 +418,7 @@
>          return NULL;
>  
>      if ( (port = xc_evtchn_alloc_unbound(self->xc_handle, dom,
remote_dom)) < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return PyInt_FromLong(port);
>  }
> @@ -425,7 +439,7 @@
>      ret = xc_physdev_pci_access_modify(
>          self->xc_handle, dom, bus, dev, func, enable);
>      if ( ret != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -447,7 +461,7 @@
>  
>      ret = xc_readconsolering(self->xc_handle, &str, &count,
clear);
>      if ( ret < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return PyString_FromStringAndSize(str, count);
>  }
> @@ -477,7 +491,7 @@
>      int i;
>      
>      if ( xc_physinfo(self->xc_handle, &info) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      *q=0;
>      for(i=0;i<sizeof(info.hw_cap)/4;i++)
> @@ -515,25 +529,25 @@
>      xen_version = xc_version(self->xc_handle, XENVER_version, NULL);
>  
>      if ( xc_version(self->xc_handle, XENVER_extraversion,
&xen_extra) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      if ( xc_version(self->xc_handle, XENVER_compile_info, &xen_cc)
!= 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      if ( xc_version(self->xc_handle, XENVER_changeset, &xen_chgset)
!= 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      if ( xc_version(self->xc_handle, XENVER_capabilities,
&xen_caps) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      if ( xc_version(self->xc_handle, XENVER_platform_parameters,
&p_parms) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      sprintf(str, "virt_start=0x%lx", p_parms.virt_start);
>  
>      xen_pagesize = xc_version(self->xc_handle, XENVER_pagesize, NULL);
>      if (xen_pagesize < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return
Py_BuildValue("{s:i,s:i,s:s,s:s,s:i,s:s,s:s,s:s,s:s,s:s,s:s}",
>                           "xen_major", xen_version >> 16,
> @@ -566,7 +580,7 @@
>          return NULL;
>     if ( xc_sedf_domain_set(self->xc_handle, domid, period,
>                             slice, latency, extratime,weight) != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -583,7 +597,7 @@
>      
>      if (xc_sedf_domain_get(self->xc_handle, domid, &period,
>                            
&slice,&latency,&extratime,&weight))
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return Py_BuildValue("{s:i,s:L,s:L,s:L,s:i,s:i}",
>                           "domain",    domid,
> @@ -611,7 +625,7 @@
>      
>      if ( xc_shadow_control(xc->xc_handle, dom, op, NULL, 0, NULL, 0,
NULL)
>           < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -641,7 +655,7 @@
>          op = XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION;
>      }
>      if ( xc_shadow_control(xc->xc_handle, dom, op, NULL, 0, &mb, 0,
NULL) < 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      mbarg = mb;
>      return Py_BuildValue("i", mbarg);
> @@ -668,7 +682,7 @@
>      sdom.cap = cap;
>  
>      if ( xc_sched_credit_domain_set(self->xc_handle, domid, &sdom)
!= 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -683,7 +697,7 @@
>          return NULL;
>      
>      if ( xc_sched_credit_domain_get(self->xc_handle, domid, &sdom)
!= 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      return Py_BuildValue("{s:H,s:H}",
>                           "weight",  sdom.weight,
> @@ -699,7 +713,7 @@
>          return NULL;
>  
>      if (xc_domain_setmaxmem(self->xc_handle, dom, maxmem_kb) != 0)
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -726,7 +740,7 @@
>      if ( xc_domain_memory_increase_reservation(self->xc_handle, dom, 
>                                                 nr_extents, extent_order, 
>                                                 address_bits, NULL) )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>      
>      Py_INCREF(zero);
>      return zero;
> @@ -748,7 +762,7 @@
>      ret = xc_domain_ioport_permission(
>          self->xc_handle, dom, first_port, nr_ports, allow_access);
>      if ( ret != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -771,7 +785,7 @@
>      ret = xc_domain_irq_permission(
>          xc->xc_handle, dom, pirq, allow_access);
>      if ( ret != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -794,7 +808,7 @@
>      ret = xc_domain_iomem_permission(
>          xc->xc_handle, dom, first_pfn, nr_pfns, allow_access);
>      if ( ret != 0 )
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -834,7 +848,7 @@
>          return NULL;
>  
>      if (fn(self->xc_handle, dom) != 0)
> -        return PyErr_SetFromErrno(xc_error);
> +        return pyxc_error_to_exception();
>  
>      Py_INCREF(zero);
>      return zero;
> @@ -1157,7 +1171,7 @@
>  PyXc_init(XcObject *self, PyObject *args, PyObject *kwds)
>  {
>      if ((self->xc_handle = xc_interface_open()) == -1) {
> -        PyErr_SetFromErrno(xc_error);
> +        pyxc_error_to_exception();
>          return -1;
>      }
>  
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

On Mon, Oct 23, 2006 at 02:17:40PM -0500, Anthony Liguori
wrote:
> Daniel P. Berrange wrote:
> >On Tue, Sep 26, 2006 at 01:58:11PM +0100, Ian Pratt wrote:
> >>>On 26/9/06 12:45, "Stephen C. Tweedie"
<sct@redhat.com> wrote:
> >>>
> >>>>>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.
> >>>>...
> >>>>
> >>>>>Any way, the upshot of all this work:
> >>>>>  # xm create error
> >>>>>  Using config file "error".
> >>>>>  Error: [2, ''Kernel ELF architecture 3 does
not match Xen
> >>>>>architecture 62'']
> >>>>IMHO this is sorely needed.  Any comments from XenSource
people?
> >>>I''d agree something like this is necessary in the
3.0.4
> >>>timeframe. I''ll let one of the guys more acquainted
with xend
> >>>comment in detail. There''s also the question of how
this will
> >>>integrate with the proposed ''XenAPI''.
> >>Yep, definitely needs to happen. However, I think we should at
least
> >>discuss alternative potentially less clunky implementation methods.
> >>Perhaps we should use a thread local errno and error string
variables?
> >
> >Attached is an update to the original patch which annotates the static
> >variables with __thread so that they are setup per-thread. It also adds
> >more informative error reporting for bad kernels.
> 
> What versions of GCC have you tested this with?
I''m doing this on FC6 - the __thread annotation is the same approach
used in libc for the per-thread errno variable, hence why I chose it
try it initially. The version are:

  glibc-2.5-3
  gcc-4.1.1-30
> I''ve found in the past that __thread can have problems when using
shared
> libraries (the exact instance was a Python binding).
I''ve not explicitly tested this integrating with the python bindings
and ensuring the correct per-thread operation.
> >The only issue is that __thread is a GCC specific extension, so
I''m not
> >sure this will work for the Solaris folks ?
> >
> >  http://gcc.gnu.org/onlinedocs/gcc/Thread_002dLocal.html
> >
> >The other option is to use the POSIX APIs  pthread_getspecific & 
> >pthread_setspecific. It''ll make the code alot more cumbersome,
but
> >would certainly be portable. I''m certainly willing to do this
though
> >if need be...
One further issue I''ve thought of is that the __thread annotation does
not provide any way to do cleanup when a thread exists. Fine if the
per-thread variable is a simple scalar, but not so good if its a char *
since I think it''ll leak memory. So I think I may have no choice by to
re-write with pthread_getspecific(), since that allows registration
of a cleanup function to free memory.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

Daniel P. Berrange wrote:
> I''m doing this on FC6 - the __thread annotation is the same
approach
> used in libc for the per-thread errno variable, hence why I chose it
> try it initially. The version are:
>
>   glibc-2.5-3
>   gcc-4.1.1-30
>
>   
>> I''ve found in the past that __thread can have problems when
using shared
>> libraries (the exact instance was a Python binding).
>>     
>
> I''ve not explicitly tested this integrating with the python
bindings
> and ensuring the correct per-thread operation.
>   
Will the python bindings actually load?  In the past, I''ve not been
able
to import a module that contains __thread variables.

Regards,

Anthony Liguori
> One further issue I''ve thought of is that the __thread annotation
does
> not provide any way to do cleanup when a thread exists. Fine if the
> per-thread variable is a simple scalar, but not so good if its a char *
> since I think it''ll leak memory. So I think I may have no choice
by to
> re-write with pthread_getspecific(), since that allows registration
> of a cleanup function to free memory.
>
> Regards,
> Dan.
>   

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

On Mon, Oct 23, 2006 at 03:04:41PM -0500, Anthony Liguori
wrote:
> Daniel P. Berrange wrote:
> >I''m doing this on FC6 - the __thread annotation is the same
approach
> >used in libc for the per-thread errno variable, hence why I chose it
> >try it initially. The version are:
> >
> >  glibc-2.5-3
> >  gcc-4.1.1-30
> >
> >  
> >>I''ve found in the past that __thread can have problems
when using shared
> >>libraries (the exact instance was a Python binding).
> >>    
> >
> >I''ve not explicitly tested this integrating with the python
bindings
> >and ensuring the correct per-thread operation.
> >  
> 
> Will the python bindings actually load?  In the past, I''ve not
been able
> to import a module that contains __thread variables.
Yes, that worked just fine. I tested using xend/xm to validate the saner
error handling behaviour for invalid kernels, eg

# xm create crash
Using config file "crash".
Error: [2, ''Invalid kernel'', "Kernel ELF type
''3'' does not match Xen type ''2''"]

# xm create crash
Using config file "crash".
Error: [2, ''Invalid kernel'', "Kernel ELF architecture
''3'' does not match Xen architecture ''62''
(x86_64)"]

# xm create crash
Using config file "crash".
Error: [2, ''Invalid kernel'', ''Not a valid ELF or raw
kernel image'']

Given the notes in the GCC docs about needing support from libdl.so,
libc.so & libthread.so I suspect one of the libraries in your stack
was not fully compliant with the __thread specs.
> >One further issue I''ve thought of is that the __thread
annotation does
> >not provide any way to do cleanup when a thread exists. Fine if the
> >per-thread variable is a simple scalar, but not so good if its a char *
> >since I think it''ll leak memory. So I think I may have no
choice by to
> >re-write with pthread_getspecific(), since that allows registration
> >of a cleanup function to free memory.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

On Mon, Oct 23, 2006 at 07:00:17PM +0100, Daniel P. Berrange wrote:
> The only issue is that __thread is a GCC specific extension, so
I''m not
> sure this will work for the Solaris folks ?
Yes, it will. We use GCC for all the Xen code itself, and __thread seems
to work fine. I''ll give your patch a sanity check tomorrow...

regards
john

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

> One further issue I''ve thought of is that the __thread annotation
does
> not provide any way to do cleanup when a thread exists. Fine if the
> per-thread variable is a simple scalar, but not so good if its a char
*
> since I think it''ll leak memory. So I think I may have no choice
by to
> re-write with pthread_getspecific(), since that allows registration
> of a cleanup function to free memory.
Do we actually need pointers? I''d much rather go with __thread.

Ian

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

On Mon, Oct 23, 2006 at 09:54:12PM +0100, Ian Pratt
wrote:
> > One further issue I''ve thought of is that the __thread
annotation does
> > not provide any way to do cleanup when a thread exists. Fine if the
> > per-thread variable is a simple scalar, but not so good if its a char
> *
> > since I think it''ll leak memory. So I think I may have no
choice by to
> > re-write with pthread_getspecific(), since that allows registration
> > of a cleanup function to free memory.
> 
> Do we actually need pointers? I''d much rather go with __thread.
Well, we need a char buffer to store the error message, since that has much
more useful info than the error code alone. I could always just do a fixed
200 byte buffer, and truncate anything longer than this which would also
actually remove the annoying OOM problem in copying the error message. 

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

On Mon, Oct 23, 2006 at 07:00:17PM +0100, Daniel P. Berrange wrote:
> Attached is an update to the original patch which annotates the static
> variables with __thread so that they are setup per-thread. It also adds
> more informative error reporting for bad kernels.
It needs merging with latest tip I think. xc_private.h is missing a
prototype for xc_set_error(), so it doesn''t compile. Also it
re-introduces ERR(), which causes problems for us, and can just be
removed.

I fixed these two problems and did some simple tests on Solaris; all
seems OK:

bash-3.00# xm create -c johnlev-64.py
Using config file "johnlev-64.py".
Error: [2, ''Invalid kernel'', "Kernel ELF architecture
''62'' does not match Xen architecture ''3''
(i386)"]

(It''s so wonderful to get a meaningful error message at last!!)

regards
john

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

On Mon, Oct 23, 2006 at 10:04:59PM +0100, John Levon
wrote:
> On Mon, Oct 23, 2006 at 07:00:17PM +0100, Daniel P. Berrange wrote:
> 
> > Attached is an update to the original patch which annotates the static
> > variables with __thread so that they are setup per-thread. It also
adds
> > more informative error reporting for bad kernels.
> 
> It needs merging with latest tip I think. xc_private.h is missing a
> prototype for xc_set_error(), so it doesn''t compile. Also it
> re-introduces ERR(), which causes problems for us, and can just be
> removed.
Ah yes, I hadn''t noticed there were xen-unstable changes in this area.
I
did the patch against 3.0.3  final release. I''ll rebase to xen-unstable
before posting again.
> I fixed these two problems and did some simple tests on Solaris; all
> seems OK:
> 
> bash-3.00# xm create -c johnlev-64.py
> Using config file "johnlev-64.py".
> Error: [2, ''Invalid kernel'', "Kernel ELF
architecture ''62'' does not match Xen architecture
''3'' (i386)"]
> 
> (It''s so wonderful to get a meaningful error message at last!!)
Thats good news, thanks for testing it.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

> > Do we actually need pointers? I''d much rather go with
__thread.
> 
> Well, we need a char buffer to store the error message, since that has
much
> more useful info than the error code alone. I could always just do a
fixed
> 200 byte buffer, and truncate anything longer than this which would
also
> actually remove the annoying OOM problem in copying the error message.
Would we be better off returning an error code and a set of parameters,
requiring a call-back into the library to get the string? 

It''s worth thinking about future language localisation here too.

Ian 

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

Ian Pratt wrote:
> Would we be better off returning an error code and a set of parameters,
> requiring a call-back into the library to get the string? 
>
> It''s worth thinking about future language localisation here too.
>   
I know it''s a bigger patch, but the Right Thing to do here is to just 
propagate an error code through the libxc functions.

The whole xc_{get,set}_error() is a cludge.  Threading wouldn''t be a 
problem if we returned proper error codes.

Looking at your patch, it doesn''t seem like it would really be that
hard...

Regards,

Anthony Liguori
> Ian 
>   

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

On Mon, Oct 23, 2006 at 10:10:31PM +0100, Ian Pratt
wrote:
> > > Do we actually need pointers? I''d much rather go with
__thread.
> > 
> > Well, we need a char buffer to store the error message, since that has
> much
> > more useful info than the error code alone. I could always just do a
> fixed
> > 200 byte buffer, and truncate anything longer than this which would
> also
> > actually remove the annoying OOM problem in copying the error message.
> 
> Would we be better off returning an error code and a set of parameters,
> requiring a call-back into the library to get the string? 
That assumes that there is a static mapping between an error code
and the error description, which there isn''t. The error description
can contain info about actual bits of metadata which were incorrect.
For example when reporting an invalid ELF architecture, it can tell
you exactly what ELF arch was found & what was expected.
> It''s worth thinking about future language localisation here too.
Yep, I don''t see any trouble adding localization to this API.
We''d just
feed the error messages through gettext before passing them into the
xc_set_error method.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

On Mon, Oct 23, 2006 at 04:14:59PM -0500, Anthony Liguori
wrote:
> Ian Pratt wrote:
> >Would we be better off returning an error code and a set of parameters,
> >requiring a call-back into the library to get the string? 
> >
> >It''s worth thinking about future language localisation here
too.
> >  
> 
> I know it''s a bigger patch, but the Right Thing to do here is to
just
> propagate an error code through the libxc functions.
> 
> The whole xc_{get,set}_error() is a cludge.  Threading wouldn''t be
a
> problem if we returned proper error codes.
This would be an insufficient level of detail compared to my patch. An error
code alone can tell you there was an invalid kernel, or even particular tests
which failed. It can *not* tell you that when the architecture mis-matched,
the expected arch was ''i386'' and the actual arch was
''x86_64''. Hence why
I provided both an error code & a detailed message. 

Notice in the following there are two strings - ''Invalid
kernel'' is the
string associated with the error code ''XC_INVALID_KERNEL''.
This is the
generic static mapping. The second string though is dynamically generated
according to the specific metadata which was incorrect - this is the 
invaluable user facing information which cuts down on debugging pain.

[root@dhcp-4-245 ~]# xm create crash
Using config file "/etc/xen/crash".
Error: [2, ''Invalid kernel'', "Kernel ELF architecture
''3'' does not match Xen architecture ''62''
(x86_64)"]
[root@dhcp-4-245 ~]# xm create crash
Using config file "/etc/xen/crash".
Error: [2, ''Invalid kernel'', "Kernel ELF type
''3'' does not match Xen type ''2''"]
[root@dhcp-4-245 ~]# xm create crash
Using config file "/etc/xen/crash".
Error: [2, ''Invalid kernel'', ''Not a valid ELF or raw
kernel image'']

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

Daniel P. Berrange wrote:
> On Mon, Oct 23, 2006 at 04:14:59PM -0500, Anthony Liguori wrote:
>   
>> Ian Pratt wrote:
>>     
>>> Would we be better off returning an error code and a set of
parameters,
>>> requiring a call-back into the library to get the string? 
>>>
>>> It''s worth thinking about future language localisation
here too.
>>>  
>>>       
>> I know it''s a bigger patch, but the Right Thing to do here is
to just
>> propagate an error code through the libxc functions.
>>
>> The whole xc_{get,set}_error() is a cludge.  Threading
wouldn''t be a
>> problem if we returned proper error codes.
>>     
>
> This would be an insufficient level of detail compared to my patch. An
error
> code alone can tell you there was an invalid kernel, or even particular
tests
> which failed. It can *not* tell you that when the architecture mis-matched,
> the expected arch was ''i386'' and the actual arch was
''x86_64''. Hence why
> I provided both an error code & a detailed message. 
>
> Notice in the following there are two strings - ''Invalid
kernel'' is the
> string associated with the error code
''XC_INVALID_KERNEL''. This is the
> generic static mapping. The second string though is dynamically generated
> according to the specific metadata which was incorrect - this is the 
> invaluable user facing information which cuts down on debugging pain.
>   
stdout from libxenctrl gets redirected to the Xend logs.  You could 
write this sort of info to the logs.

I don''t know of many users that will be able to make sense out of the 
following lines.  How many users know what an "ELF architecture" is
that
wouldn''t be capable of looking in log file?

If a user passes an invalid kernel line in the config, I think an 
appropriate error would be "Kernel <filename> is not a valid Xen
kernel."

That''s pretty clear and understandable.  This message is totally 
reproducible in xm with just an XC_INVALID_KERNEL error code.

Regards,

Anthony Liguori
> [root@dhcp-4-245 ~]# xm create crash
> Using config file "/etc/xen/crash".
> Error: [2, ''Invalid kernel'', "Kernel ELF
architecture ''3'' does not match Xen architecture
''62'' (x86_64)"]
> [root@dhcp-4-245 ~]# xm create crash
> Using config file "/etc/xen/crash".
> Error: [2, ''Invalid kernel'', "Kernel ELF type
''3'' does not match Xen type ''2''"]
> [root@dhcp-4-245 ~]# xm create crash
> Using config file "/etc/xen/crash".
> Error: [2, ''Invalid kernel'', ''Not a valid ELF or
raw kernel image'']
>
> Dan.
>   

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

On Mon, Oct 23, 2006 at 04:38:54PM -0500, Anthony Liguori wrote:
> stdout from libxenctrl gets redirected to the Xend logs.  You could 
> write this sort of info to the logs.
We already have enough problems with stuff disappearing into the logs
and having to look through it carefully to find the actual problem :(
> I don''t know of many users that will be able to make sense out of
the
> following lines.  How many users know what an "ELF architecture"
is that
> wouldn''t be capable of looking in log file?
The messages could use some loving certainly.
> If a user passes an invalid kernel line in the config, I think an 
> appropriate error would be "Kernel <filename> is not a valid Xen
kernel."
Throwing away information on the error never seems like a good idea,
especially when you consider, say, the Xen API. Logs are particular
awkward when you''re remotely managing a machine via a VM console. Even
finding the right host machine could be frustrating enough.

regards
john

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

On Mon, Oct 23, 2006 at 04:38:54PM -0500, Anthony Liguori
wrote:
> Daniel P. Berrange wrote:
> >On Mon, Oct 23, 2006 at 04:14:59PM -0500, Anthony Liguori wrote:
> >  
> >>Ian Pratt wrote:
> >>    
> >>>Would we be better off returning an error code and a set of
parameters,
> >>>requiring a call-back into the library to get the string? 
> >>>
> >>>It''s worth thinking about future language localisation
here too.
> >>> 
> >>>      
> >>I know it''s a bigger patch, but the Right Thing to do here
is to just
> >>propagate an error code through the libxc functions.
> >>
> >>The whole xc_{get,set}_error() is a cludge.  Threading
wouldn''t be a
> >>problem if we returned proper error codes.
> >>    
> >
> >This would be an insufficient level of detail compared to my patch. An 
> >error
> >code alone can tell you there was an invalid kernel, or even particular
> >tests
> >which failed. It can *not* tell you that when the architecture
mis-matched,
> >the expected arch was ''i386'' and the actual arch was
''x86_64''. Hence why
> >I provided both an error code & a detailed message. 
> >
> >Notice in the following there are two strings - ''Invalid
kernel'' is the
> >string associated with the error code
''XC_INVALID_KERNEL''. This is the
> >generic static mapping. The second string though is dynamically
generated
> >according to the specific metadata which was incorrect - this is the 
> >invaluable user facing information which cuts down on debugging pain.
> >  
> 
> stdout from libxenctrl gets redirected to the Xend logs.  You could 
> write this sort of info to the logs.
Where is it utterly inaccessible for any applications talking to XenD.
> I don''t know of many users that will be able to make sense out of
the
> following lines.  How many users know what an "ELF architecture"
is that
> wouldn''t be capable of looking in log file?
You don''t neccessarily show that level of detail to the user, but it is
important to propagate it all the way back to the tools talking to libxc
or Xend. xm, or libvirt, or virt-manager, or any other tool talking to
XenD should have access to this kind of debugging info. If I''m doing
off-box management of hosts & a domain create operation fails, I
don''t
want to have to login to the box in question to get this information
out of the xend logs, when xend could quite easily have passed it all
back over the wire in the first place.
> If a user passes an invalid kernel line in the config, I think an 
> appropriate error would be "Kernel <filename> is not a valid Xen
kernel."
> 
> That''s pretty clear and understandable.  This message is totally 
> reproducible in xm with just an XC_INVALID_KERNEL error code.
It really depends on the end user in question. If the information is 
available, an API should pass it back to the applicationxs to decide what
level of detail is appropriate to present. We shouldn''t encode the 
error reporting policy in the API itself, because it has no idea of the
needs of the caller.  

BTW, when I say ''user'' i mean caller of the (Xend) APIs (ie
applications),
rather than human operators.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

> > Would we be better off returning an error code and a set of
parameters,
> > requiring a call-back into the library to get the string?
> 
> That assumes that there is a static mapping between an error code
> and the error description, which there isn''t. The error
description
> can contain info about actual bits of metadata which were incorrect.
> For example when reporting an invalid ELF architecture, it can tell
> you exactly what ELF arch was found & what was expected.
That''s precisely my point. The various bits of metadata can be
parameters passed back along with the error code. It''s possible that
some callers will be able to do things with the metadata that are more
useful than just printing the string. I guess the error parameters could
be returned as a va_list. Many callers would just call back into the
library to get the appropriate error string.


Ian

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

On Mon, Oct 23, 2006 at 11:15:21PM +0100, Ian Pratt
wrote:
> > > Would we be better off returning an error code and a set of
> parameters,
> > > requiring a call-back into the library to get the string?
> > 
> > That assumes that there is a static mapping between an error code
> > and the error description, which there isn''t. The error
description
> > can contain info about actual bits of metadata which were incorrect.
> > For example when reporting an invalid ELF architecture, it can tell
> > you exactly what ELF arch was found & what was expected.
> 
> That''s precisely my point. The various bits of metadata can be
> parameters passed back along with the error code. It''s possible
that
> some callers will be able to do things with the metadata that are more
> useful than just printing the string. I guess the error parameters could
> be returned as a va_list. Many callers would just call back into the
> library to get the appropriate error string.
Ahh, I mis-read your comment a little. That is certainly one possibility
although I''m not entirely a fan of va_list as an API - perhaps either a
struct with a handful of generic data fields for returning metadata, or
a union switched on the error code would work - it''d be nice to be able
to copy all the bits around without needing memory allocation in any of
the error handling paths.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

Anthony Liguori wrote:
> I know it''s a bigger patch, but the Right Thing to do here is to
just
> propagate an error code through the libxc functions.
That doesn''t allow a descriptive text message, which is often quite
useful to figure what went wrong in detail.  error codes such as errno
== EINVAL are not very helpful for trouble shooting ...

What number space we are talking about btw?  errno?  something else?

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

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

On 24/10/06 08:47, "Gerd Hoffmann" <kraxel@suse.de> wrote:
>> I know it''s a bigger patch, but the Right Thing to do here is
to just
>> propagate an error code through the libxc functions.
> 
> That doesn''t allow a descriptive text message, which is often
quite
> useful to figure what went wrong in detail.  error codes such as errno
> == EINVAL are not very helpful for trouble shooting ...
> 
> What number space we are talking about btw?  errno?  something else?
I think a separate numbering space, where each err number would have some
number of auxiliary arguments is being suggested, probably with an
xc_{get,set}_errnum() interface (since the ''errnum'' would not
be a simple
scalar so would be a bit of a pain to pass around). xc_{get,set}_errtext()
(or whatever it is called) could co-exist with this, so I am personally in
favour of the patch, at least as a starting point for further work. It
directly tackles our biggest current err-handling problem, which is stupidly
throwing away useful diagnostic info (or writing it to log files where it
gets hidden in with piles of other crap).

I guess using __thread is the best of a bad set of choices, by the way.
It''ll limit us to gcc-3.3.x and later, which is fine. And
we''ll want to
build 32-bit with -mno-tls-direct-seg-refs. We can add that to CFLAGS for
all tools. I was wondering about explicitly stating that libxc is no
thread-safe for any given ''xc handle'', but actually that is
just storing up
bigger trouble.

 -- Keir



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

On Mon, Oct 23, 2006 at 10:47:40PM +0100, John Levon
wrote:
> On Mon, Oct 23, 2006 at 04:38:54PM -0500, Anthony Liguori wrote:
> > If a user passes an invalid kernel line in the config, I think an 
> > appropriate error would be "Kernel <filename> is not a
valid Xen kernel."
> 
> Throwing away information on the error never seems like a good idea,
> especially when you consider, say, the Xen API. Logs are particular
> awkward when you''re remotely managing a machine via a VM console.
Even
> finding the right host machine could be frustrating enough.
  I agree totally. You can have structured information where you need
it if reported at the API level. Saving in unstructured logs on the
managed machine is in general not very helpful.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

Keir Fraser wrote:
> On 24/10/06 08:47, "Gerd Hoffmann" <kraxel@suse.de> wrote:
> 
>> What number space we are talking about btw?  errno?  something else?
> 
> I think a separate numbering space, where each err number would have some
> number of auxiliary arguments is being suggested, probably with an
> xc_{get,set}_errnum() interface (since the ''errnum'' would
not be a simple
> scalar so would be a bit of a pain to pass around).
I somehow feel this is a bit overdesigned.  I''ve just walked through my
domain builder rewrite, trying to assign useful error codes.  After all
there are not that many different cases:

 (1) internal errors (all sorts of sanity checks which normally
     never ever fail except when coding new bugs ;) ).
 (2) out of memory.
 (3) reading some file failed (kernel / initrd / hvmloader / whatelse).
 (4) invalid kernel image (broken elf headers, truncated file, ...).
 (5) incompatible kernel image (pae vs. nonpae, ppc kernel on x86, ...).
 (6) some invalid parameter (such as asking for a feature not supported
     by hypervisor or guest kernel, ...)

And the is only one where a fixed set of parameters specifying in detail
what went wrong somehow makes sense: for #3 this would be the filename
and maybe the reason (EPERM, ENOENT, ...).  For the other ones IMHO only
free text makes sense as detailed description ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

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