Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
> pointer to NULL-initialized pointer, or pointer to error_abort or
> error_fatal, for callee to report error.

Yes.

> But very few functions instead get Error **errp as IN-argument:
> it's assumed to be set, and callee should clean it.

What do you mean by "callee should clean"?  Let's see below.

> In such cases, rename errp to errp_in.

I acknowledge that errp arguments that don't have the usual meaning can
be confusing.

Naming can help, comments can help, but perhaps we can tweak the code to
avoid the problem instead.  Let's see:

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/monitor/hmp.h |  2 +-
>  include/qapi/error.h  |  2 +-
>  ui/vnc.h              |  2 +-
>  monitor/hmp-cmds.c    |  8 ++++----
>  ui/vnc.c              | 10 +++++-----
>  util/error.c          |  8 ++++----
>  6 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index a0e9511440..f929814f1a 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -16,7 +16,7 @@
>  
>  #include "qemu/readline.h"
>  
> -void hmp_handle_error(Monitor *mon, Error **errp);
> +void hmp_handle_error(Monitor *mon, Error **errp_in);
>  
>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>  void hmp_info_version(Monitor *mon, const QDict *qdict);
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..9376f59c35 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -283,7 +283,7 @@ void error_free(Error *err);
>  /*
>   * Convenience function to assert that *@errp is set, then silently free it.
>   */
> -void error_free_or_abort(Error **errp);
> +void error_free_or_abort(Error **errp_in);
>  
>  /*
>   * Convenience function to warn_report() and free @err.
> diff --git a/ui/vnc.h b/ui/vnc.h
> index fea79c2fc9..00e0b48f2f 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
>  
>  /* Protocol stage functions */
>  void vnc_client_error(VncState *vs);
> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
>  
>  void start_client_init(VncState *vs);
>  void start_auth_vnc(VncState *vs);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index b2551c16d1..941d5d0a45 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -60,11 +60,11 @@
>  #include <spice/enums.h>
>  #endif
>  
> -void hmp_handle_error(Monitor *mon, Error **errp)
> +void hmp_handle_error(Monitor *mon, Error **errp_in)
>  {
> -    assert(errp);
> -    if (*errp) {
> -        error_reportf_err(*errp, "Error: ");
> +    assert(errp_in);
> +    if (*errp_in) {
> +        error_reportf_err(*errp_in, "Error: ");
>      }
>  }

This functions frees the error.  It leaves nothing for the caller to
clean up.

All callers pass &ERR, where ERR is a local variable.  Perhaps a more
robust way to signal "@errp is not the usual out-argument" would be
peeling off an indirection: pass ERR, drop the assertion.

>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 87b8045afe..9d6384d9b1 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
>      g_free(vs);
>  }
>  
> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
>  {
>      if (ret <= 0) {
>          if (ret == 0) {
> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>              vnc_disconnect_start(vs);
>          } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>              trace_vnc_client_io_error(vs, vs->ioc,
> -                                      errp ? error_get_pretty(*errp) :
> +                                      errp_in ? error_get_pretty(*errp_in) :
>                                        "Unknown");
>              vnc_disconnect_start(vs);
>          }
>  
> -        if (errp) {
> -            error_free(*errp);
> -            *errp = NULL;
> +        if (errp_in) {
> +            error_free(*errp_in);
> +            *errp_in = NULL;
>          }
>          return 0;
>      }

This function isn't trivial, and lacks a contract, so let's figure out
what it does and how it's used.

@ret can be:

* Zero

  Trace EOF, call vnc_disconnect_start(), free the error, return zero.

  Aside: freeing the error without looking at it feels odd.  Can this
  happen?

* Negative other than QIO_CHANNEL_ERR_BLOCK

  Trace the error if any, else "Unknown" error, call
  vnc_disconnect_start(), free the error if any, return zero.

  Note that we can't have errp && !*errp here, or else tracing crashes
  in error_get_pretty().

* QIO_CHANNEL_ERR_BLOCK

  Free the error, return zero

* Positive

  Do nothing, return @ret

Callers pass one of the following:

* ret = -1 and errp = NULL

  This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK".  Since
  error is null, it traces an "Unknown" error.

* ret and &err, where ret = FUN(..., &err), and FUN is
  qio_channel_read() or qio_channel_write().

  qio_channel_read(), _write() are documented to return non-negative on
  success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other
  error.  By convention, they set an error exactly when they fail,
  i.e. when they return a negative value.

  When qio_channel_read() / _write() succeed, we use case "Positive" or
  "Zero".  We don't free the error, which is fine, as none was returned.
  Aside: I *guess* the channel is non-blocking, and "zero" can happen
  only when read hits EOF.

  When qio_channel_read() / _write() fail, we use one of the error
  cases.

Looks like vnc_client_io_error() takes an error code @ret and an
optional error object in @errp with additional details.  If @ret is
non-negative, @errp must be null or point to null.  If @ret is negative,
@errp must be null or point to non-null.

vnc_client_io_error() frees the error.  It leaves nothing for the caller
to clean up.

I think we can again peel off an indirection.  The two kinds of calls
become:

* ret = -1 and err = NULL

  No textual change, but the NULL gets converted to Error * instead of
  Error **.

* ret and err

  Pass the (possibly null) error object instead of a pointer to the
  local variable.

> diff --git a/util/error.c b/util/error.c
> index d4532ce318..b3ff3832d6 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -271,11 +271,11 @@ void error_free(Error *err)
>      }
>  }
>  
> -void error_free_or_abort(Error **errp)
> +void error_free_or_abort(Error **errp_in)
>  {
> -    assert(errp && *errp);
> -    error_free(*errp);
> -    *errp = NULL;
> +    assert(errp_in && *errp_in);
> +    error_free(*errp_in);
> +    *errp_in = NULL;
>  }
>  
>  void error_propagate(Error **dst_errp, Error *local_err)

This functions frees the error.  It leaves nothing for the caller to
clean up.

All callers pass &ERR, where ERR is a local variable.  We can peel off
an indirection.


I figure your commit message's "But very few functions instead get Error
**errp as IN-argument: it's assumed to be set, and callee should clean
it" is to be read as "a few functions take Error **errp as IN-argument,
and free it".

You found three instances of confusing Error **errp.  How?  I'm asking
because I wonder whether there are more.

We can avoid the confusing Error **errp in all three cases by peeling
off an indirection.  What do you think?