error info handling in MIT krb5 code

[Ken Raeburn]

So, I'm getting around to adding to our error-handling support as was
discussed on one of the mailing lists a long time ago.  I took a look
at what Heimdal is doing, and I like it.

These are the Heimdal interfaces in 0.7.2:

krb5_error_code krb5_set_error_string(krb5_context, const char *, ...)
krb5_error_code krb5_vset_error_string(krb5_context, const char *,  
va_list)
     Calls vasprintf to allocate and format the message, or if that
     fails, uses vsnprintf and the preallocated per-context error_buf.
     Stores the pointer to the formatted message into error_string.
char *krb5_get_error_string(krb5_context)
     Returns the error_string field value, after clearing it, so only
     one call can be made after any error.
krb5_boolean krb5_have_error_string(krb5_context)
     Indicates whether error_string is non-null.
void krb5_free_error_string(krb5_context, char *)
     Frees the supplied string if it's not the error_buf of the
     context.
void krb5_clear_error_string(krb5_context)
     Sets the context's error_string to NULL.  If it was non-null and
     didn't match the error_buf, free it, too.

New in snapshots:

char *krb5_get_error_message(krb5_context, krb5_error_code)
     Returns the error_string if any; otherwise, tries the error
     message for the supplied error code; if none, formats an "unknown
     error" message.  Always returns NULL or heap storage that the
     caller should eventually free.

Doug's suggestion: Also store the error code, and if asking for a
message associated with a *different* error code, throw away the
stored data.  Also look at OpenSSL ERR_* routines.


I took a look at OpenSSL.  They store file and line number where the
error is raised; that's possibly interesting for debugging.  They
maintain what sounds like a FIFO queue of errors, and have a routine
which prints out all the errors in the queue.  I'm not convinced
that's better than having each site where an error code is triggered
by an earlier error do something like:

   krb5_set_error(ctx, new_code, "Can't open credentials cache %s: %s",
                  cachename, krb5_get_error(ctx, underlying_error_code))

(well, aside from memory management issues)

Without something like this, I think you're just stuck printing a
series of messages in a particular order with some chosen separator
string like ":\n" in between.  I'm certainly willing to be convinced.
Either way, we need an approach that's amenable to
internationalization.


So, I'd propose we do something like this:

void krb5_set_error(krb5_context, krb5_error_code, const char *, ...)
void krb5_vset_error(krb5_context, krb5_error_code, const char *,  
va_list)
     Formats a message in newly allocated storage and saves it in the
     context, along with the error code associated with it.  If
     allocation fails, uses pre-allocated storage associated with (in?)
     the context.

     If we go with *printf formatting, the implementation is pretty
     easy on most platforms.  If we want to get fancy instead, we could
     do our own formatting, and add a new format code which takes a
     krb5_error_code argument, uses it to fetch the previous error
     message from the current context, and substitutes the resulting
     string, freeing it up after use.  That would side-step the little
     memory management issue I glossed over above.  It would take a bit
     longer, but it may be more convenient, and I've written such code
     before; there are one or two instances in the krb5 code already.

char *krb5_get_error(krb5_context, krb5_error_code)
     If the supplied error code matches the saved one, returns the most
     recently stored message, and clears the pointer in the context
     (thus leaving the caller with the only reference to the allocated
     storage).

     If the two error codes don't match, returns a copy of the error
     string associated with the supplied error code (again, leaving the
     caller with the only reference *or* a reference to the no-memory
     scratch buffer), and clears out any saved value.

     Cannot return NULL.

     Note that in the case of two out-of-memory errors, if the handle
     to the no-memory buffer has been retrieved twice and never
     "freed", the first-retrieved error message may change.  Thus, the
     caller should retrieve and use one error message before retrieving
     the next.

     This is similar to Heimdal's krb5_get_error_message, but compares
     the supplied error code to the saved one, rather than only using
     it as a fallback.

void krb5_free_error(krb5_context, char *)
     Frees up the storage if it's not the special no-memory buffer
     associated with the context.

void krb5_clear_error(krb5_context)
     Frees up the message directly.

Some are equivalent to the Heimdal ones with different names, but I
wanted consistent names, and I didn't necessarily want to use the
Heimdal name when I'm changing the interfaces slightly.

All these function names should be in the krb5 library, though we
might stick helper functions in the support library.  Note that the
crypto library may need to store error info, but need not use the
above interface.

Q: Custom or *printf formatting?

Q: Should there be a get_error version that doesn't require having the
correct numeric error code?  Or maybe a magic value (maybe 0 or -1)
which matches any saved code?

Q: How about a "peek" function that doesn't reset the state?

Q: Should we save file name and line number?  Function name?  The
above interfaces could be macros that wrap calls to functions taking
the extra arguments.  They might help in debugging, but currently
there's no way to retrieve the information.  (Perhaps an environment
variable could cause krb5_set_error to include them in the generated
string.  In a locale-dependent form, of course.)

Ken