Re: [Qemu-devel] [PATCH v2 1/3] error: don't rely on pointer comparisons

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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.