"Michael S. Tsirkin" <m...@redhat.com> writes:
> makes it possible to copy error_abort pointers,
> not just pass them on directly.
Humor me, and start your sentences with a capital letter :)
> This is needed because follow-up patches add support for
> Error *local_err = ...;
> as a way to set an abort-on-error pointer, which requires that we have
> more than just a global error_abort abort-on-error pointer, but that any
> number of pointers all resolve to something specific.
>
> Add an assert statement when class is retrieved, to make sure we still
> get a core-dump if we (somehow) attempt to output the abort errp by
> mistake.
Description could be clearer, but let's discuss the actual patches
first.
>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
> ---
> util/error.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/util/error.c b/util/error.c
> index 14f4351..e10cb34 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -20,7 +20,13 @@ struct Error
> ErrorClass err_class;
> };
>
> -Error *error_abort;
> +static Error error_abort_st = { .err_class = ERROR_CLASS_MAX };
> +Error *error_abort = &error_abort_st;
> +
> +static bool error_is_abort(Error **errp)
> +{
> + return errp && *errp == error_abort;
> +}
If anything changes the value of error_abort, we're now screwed.
>
> void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> {
> @@ -40,7 +46,7 @@ void error_set(Error **errp, ErrorClass err_class, const
> char *fmt, ...)
> va_end(ap);
> err->err_class = err_class;
>
> - if (errp == &error_abort) {
> + if (error_is_abort(errp)) {
> error_report_err(err);
> abort();
> }
> @@ -76,7 +82,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass
> err_class,
> va_end(ap);
> err->err_class = err_class;
>
> - if (errp == &error_abort) {
> + if (error_is_abort(errp)) {
> error_report_err(err);
> abort();
> }
> @@ -121,7 +127,7 @@ void error_set_win32(Error **errp, int win32_err,
> ErrorClass err_class,
> va_end(ap);
> err->err_class = err_class;
>
> - if (errp == &error_abort) {
> + if (error_is_abort(errp)) {
> error_report_err(err);
> abort();
> }
> @@ -144,6 +150,7 @@ Error *error_copy(const Error *err)
>
> ErrorClass error_get_class(const Error *err)
> {
> + assert(err->err_class < ERROR_CLASS_MAX);
The assertion makes some sense independent of the rest of this series.
It's not as tight as it could be when the compiler makes ErrorClass
signed.
> return err->err_class;
> }
>
> @@ -168,7 +175,7 @@ void error_free(Error *err)
>
> void error_propagate(Error **dst_errp, Error *local_err)
> {
> - if (local_err && dst_errp == &error_abort) {
> + if (local_err && error_is_abort(dst_errp)) {
> error_report_err(local_err);
> abort();
> } else if (dst_errp && !*dst_errp) {
As Eric pointed out, this isn't quite right.
Your use of ERROR_CLASS_MAX is unobvious, and needs an explanatory
comment somewhere. I'd put it right next to its definition if it wasn't
defined implicitly.
