Re: [Qemu-devel] [PATCH v2 6/7] error: Revamp interface documentation

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/qapi/error.h | 177 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 127 insertions(+), 50 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 8c3a7dd..7d808e8 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -2,13 +2,75 @@
>   * QEMU Error Objects
>   *
>   * Copyright IBM, Corp. 2011
> + * Copyright (C) 2011-2015 Red Hat, Inc.
>   *
>   * Authors:
>   *  Anthony Liguori   <aliguori@us.ibm.com>
> + *  Markus Armbruster <armbru@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2.  See
>   * the COPYING.LIB file in the top-level directory.
>   */
> +
> +/*
> + * Error reporting system loosely patterned after Glib's GError.

Excellent; it's great to have this documented.
Do we have a note anywhere that says why we don't just use GError?

Dave

> + *
> + * Create an error:
> + *     error_setg(&err, "situation normal, all fouled up");
> + *
> + * Report an error to stderr:
> + *     error_report_err(err);
> + * This frees the error object.
> + *
> + * Report an error somewhere else:
> + *     const char *msg = error_get_pretty(err);
> + *     do with msg what needs to be done...
> + *     error_free(err);
> + *
> + * Handle an error without reporting it (just for completeness):
> + *     error_free(err);
> + * 
> + * Pass an existing error to the caller:
> + *     error_propagate(errp, err);
> + * where Error **errp is a parameter, by convention the last one.
> + *
> + * Create a new error and pass it to the caller:
> + *     error_setg(errp, "situation normal, all fouled up");
> + *
> + * Call a function and receive an error from it:
> + *     Error *err = NULL;
> + *     foo(arg, &err);
> + *     if (err) {
> + *         handle the error...
> + *     }
> + *
> + * Call a function ignoring errors:
> + *     foo(arg, NULL);
> + *
> + * Call a function aborting on errors:
> + *     foo(arg, &error_abort);
> + *
> + * Receive an error and pass it on to the caller:
> + *     Error *err = NULL;
> + *     foo(arg, &err);
> + *     if (err) {
> + *         handle the error...
> + *         error_propagate(errp, err);
> + *     }
> + * where Error **errp is a parameter, by convention the last one.
> + *
> + * Do *not* "optimize" this to
> + *     foo(arg, errp);
> + *     if (*errp) { // WRONG!
> + *         handle the error...
> + *     }
> + * because errp may be NULL!
> + *
> + * But when all you do with the error is pass it on, please use
> + *     foo(arg, errp);
> + * for readability.
> + */
> +
>  #ifndef ERROR_H
>  #define ERROR_H
>  
> @@ -16,85 +78,100 @@
>  #include "qapi-types.h"
>  #include <stdbool.h>
>  
> -/**
> - * A class representing internal errors within QEMU.  An error has a ErrorClass
> - * code and a human message.
> +/*
> + * Opaque error object.
>   */
>  typedef struct Error Error;
>  
> -/**
> - * Set an indirect pointer to an error given a ErrorClass value and a
> - * printf-style human message.  This function is not meant to be used outside
> - * of QEMU.
> +/*
> + * Get @err's human-readable error message.
>   */
> -void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> -    GCC_FMT_ATTR(3, 4);
> +const char *error_get_pretty(Error *err);
>  
> -/**
> - * Set an indirect pointer to an error given a ErrorClass value and a
> - * printf-style human message, followed by a strerror() string if
> - * @os_error is not zero.
> +/*
> + * Get @err's error class.
> + * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
> + * strongly discouraged.
> + */
> +ErrorClass error_get_class(const Error *err);
> +
> +/*
> + * Create a new error object and assign it to *@errp.
> + * If @errp is NULL, the error is ignored.  Don't bother creating one
> + * then.
> + * If @errp is &error_abort, print a suitable message and abort().
> + * If @errp is anything else, *@errp must be NULL.
> + * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
> + * human-readable error message is made from printf-style @fmt, ...
> + */
> +void error_setg(Error **errp, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +
> +/*
> + * Just like error_setg(), with @os_error info added to the message.
> + * If @os_error is non-zero, ": " + strerror(os_error) is appended to
> + * the human-readable error message.
>   */
>  void error_setg_errno(Error **errp, int os_error, const char *fmt, ...)
>      GCC_FMT_ATTR(3, 4);
>  
>  #ifdef _WIN32
> -/**
> - * Set an indirect pointer to an error given a ErrorClass value and a
> - * printf-style human message, followed by a g_win32_error_message() string if
> - * @win32_err is not zero.
> +/*
> + * Just like error_setg(), with @win32_error info added to the message.
> + * If @win32_error is non-zero, ": " + g_win32_error_message(win32_err)
> + * is appended to the human-readable error message.
>   */
>  void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
>      GCC_FMT_ATTR(3, 4);
>  #endif
>  
> -/**
> - * Same as error_set(), but sets a generic error
> +/*
> + * Propagate error object (if any) from @local_err to @dst_errp.
> + * If @local_err is NULL, do nothing (because there's nothing to
> + * propagate).
> + * Else, if @dst_errp is NULL, errors are being ignored.  Free the
> + * error object.
> + * Else, if @dst_errp is &error_abort, print a suitable message and
> + * abort().
> + * Else, if @dst_errp already contains an error, ignore this one: free
> + * the error object.
> + * Else, move the error object from @local_err to *@dst_errp.
> + * On return, @local_err is invalid.
>   */
> -void error_setg(Error **errp, const char *fmt, ...)
> -    GCC_FMT_ATTR(2, 3);
> +void error_propagate(Error **dst_errp, Error *local_err);
>  
> -/**
> - * Helper for open() errors
> +/*
> + * Convenience function to report open() failure.
>   */
>  void error_setg_file_open(Error **errp, int os_errno, const char *filename);
>  
>  /*
> - * Get the error class of an error object.
> - */
> -ErrorClass error_get_class(const Error *err);
> -
> -/**
> - * Returns an exact copy of the error passed as an argument.
> + * Return an exact copy of @err.
>   */
>  Error *error_copy(const Error *err);
>  
> -/**
> - * Get a human readable representation of an error object.
> - */
> -const char *error_get_pretty(Error *err);
> -
> -/**
> - * Convenience function to error_report() and free an error object.
> - */
> -void error_report_err(Error *);
> -
> -/**
> - * Propagate an error to an indirect pointer to an error.  This function will
> - * always transfer ownership of the error reference and handles the case where
> - * dst_err is NULL correctly.  Errors after the first are discarded.
> - */
> -void error_propagate(Error **dst_errp, Error *local_err);
> -
> -/**
> - * Free an error object.
> +/*
> + * Free @err.
> + * @err may be NULL.
>   */
>  void error_free(Error *err);
>  
> -/**
> - * If passed to error_set and friends, abort().
> +/*
> + * Convenience function to error_report() and free @err.
>   */
> +void error_report_err(Error *);
>  
> +/*
> + * Just like error_setg(), except you get to specify the error class.
> + * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
> + * strongly discouraged.
> + */
> +void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> +    GCC_FMT_ATTR(3, 4);
> +
> +/*
> + * Pass to error_setg() & friends to abort() on error.
> + */
>  extern Error *error_abort;
>  
>  #endif
> -- 
> 1.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK