Re: [Xen-devel] [RFC v2 7/9] Use auto-propagated errp

[Eric Blake <eblake@xxxxxxxxxx>]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> This commit is generated by command
> 
> git grep -l 'Error \*\*errp' | while read f; \
> do spatch --sp-file \
> scripts/coccinelle/auto-propagated-errp.cocci --in-place $f; done
> 

As mentioned in your cover letter, this fails syntax-check and
compilation without squashing in some followups; if we can't improve the
.cocci script to do it automatically, then manually squashing in
cleanups (and documenting what types of cleanups they were) is fine.
(The goal for a mechanical patch like this is to make it easy enough to
automate downstream, even where the file contents are changed, but where
the process for creating those changes are the same).

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
> ---

Spot-checking

>  block/io.c                          |  11 +-

>  block/nbd.c                         |  44 +++---

>  qapi/qapi-visit-core.c              |  53 ++-----

just to see how it looks.

> +++ b/block/io.c
> @@ -136,7 +136,6 @@ static void bdrv_merge_limits(BlockLimits *dst, const 
> BlockLimits *src)
>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    Error *local_err = NULL;
>  

Umm, no insertion of ERR_FUNCTION_BEGIN().  Oops.

>      memset(&bs->bl, 0, sizeof(bs->bl));
>  
> @@ -151,9 +150,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
> **errp)
>  
>      /* Take some limits from the children as a default */
>      if (bs->file) {
> -        bdrv_refresh_limits(bs->file->bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        bdrv_refresh_limits(bs->file->bs, errp);
> +        if (*errp) {
>              return;
>          }
>          bdrv_merge_limits(&bs->bl, &bs->file->bs->bl);
> @@ -166,9 +164,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
> **errp)
>      }
>  
>      if (bs->backing) {
> -        bdrv_refresh_limits(bs->backing->bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        bdrv_refresh_limits(bs->backing->bs, errp);
> +        if (*errp) {
>              return;

Rest of the changes in this file are good if the macro gets added correctly.

>          }
>          bdrv_merge_limits(&bs->bl, &bs->backing->bs->bl);

> +++ b/block/nbd.c
> @@ -808,7 +808,6 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, 
> uint64_t handle,
>      NBDReplyChunkIter iter;
>      NBDReply reply;
>      void *payload = NULL;
> -    Error *local_err = NULL;

Recurring problem of not inserting the macro as expected.

>  
>      NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
>                              qiov, &reply, &payload)
> @@ -827,20 +826,20 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState 
> *s, uint64_t handle,
>              break;
>          case NBD_REPLY_TYPE_OFFSET_HOLE:
>              ret = nbd_parse_offset_hole_payload(s, &reply.structured, 
> payload,
> -                                                offset, qiov, &local_err);
> +                                                offset, qiov, errp);
>              if (ret < 0) {
>                  nbd_channel_error(s, ret);
> -                nbd_iter_channel_error(&iter, ret, &local_err);
> +                nbd_iter_channel_error(&iter, ret, errp);
>              }
>              break;
>          default:
>              if (!nbd_reply_type_is_error(chunk->type)) {
>                  /* not allowed reply type */
>                  nbd_channel_error(s, -EINVAL);
> -                error_setg(&local_err,
> +                error_setg(errp,
>                             "Unexpected reply type: %d (%s) for CMD_READ",

Could almost fold these lines (but I'm not asking you to; keeping this
patch as mechanical as possible is fine).

>                             chunk->type, nbd_reply_type_lookup(chunk->type));
> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
> +                nbd_iter_channel_error(&iter, -EINVAL, errp);
>              }
>          }
>  
> @@ -861,7 +860,6 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState 
> *s,
>      NBDReplyChunkIter iter;
>      NBDReply reply;
>      void *payload = NULL;
> -    Error *local_err = NULL;
>      bool received = false;

Oops on the macro.

> @@ -1174,15 +1172,13 @@ static QIOChannelSocket 
> *nbd_establish_connection(SocketAddress *saddr,
>                                                    Error **errp)
>  {
>      QIOChannelSocket *sioc;
> -    Error *local_err = NULL;
>  
>      sioc = qio_channel_socket_new();
>      qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client");
>  
> -    qio_channel_socket_connect_sync(sioc, saddr, &local_err);
> -    if (local_err) {
> +    qio_channel_socket_connect_sync(sioc, saddr, errp);
> +    if (*errp) {
>          object_unref(OBJECT(sioc));
> -        error_propagate(errp, local_err);
>          return NULL;

But getting rid of error_propagate() is nice.

Did you grep for instances of error_propagate() after your mechanical
patch, to see what else the Coccinelle script might have missed?


> +++ b/qapi/opts-visitor.c
> @@ -275,6 +275,7 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
>  static void
>  opts_check_list(Visitor *v, Error **errp)
>  {
> +     ERRP_FUNCTION_BEGIN();
>      /*
>       * Unvisited list elements will be reported later when checking
>       * whether unvisited struct members remain.

Here the macro got added, but with no obvious benefit later on (although
we also argued that adding it even when it makes no difference is not
bad, if that's easier to automate for style checking).

> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index d192724b13..3ee4c7a2e7 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -25,6 +25,7 @@ struct QapiDeallocVisitor
>  static void qapi_dealloc_start_struct(Visitor *v, const char *name, void 
> **obj,
>                                        size_t unused, Error **errp)
>  {
> +     ERRP_FUNCTION_BEGIN();
>  }

Here's an example where exempting empty functions would be nicer.


> +++ b/qapi/qapi-visit-core.c
> @@ -39,18 +39,15 @@ void visit_free(Visitor *v)
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp)
>  {
> -    Error *err = NULL;
> -

Oops, macro not added.

>      trace_visit_start_struct(v, name, obj, size);
>      if (obj) {
>          assert(size);
>          assert(!(v->type & VISITOR_OUTPUT) || *obj);
>      }
> -    v->start_struct(v, name, obj, size, &err);
> +    v->start_struct(v, name, obj, size, errp);
>      if (obj && (v->type & VISITOR_INPUT)) {
> -        assert(!err != !*obj);
> +        assert(!*errp != !*obj);
>      }
> -    error_propagate(errp, err);
>  }

But the cleanup is sane, once the macro is present.

> @@ -152,12 +143,10 @@ void visit_type_int(Visitor *v, const char *name, 
> int64_t *obj, Error **errp)
>  static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
>                               uint64_t max, const char *type, Error **errp)
>  {
> -    Error *err = NULL;
>      uint64_t value = *obj;
>  
> -    v->type_uint64(v, name, &value, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    v->type_uint64(v, name, &value, errp);
> +    if (*errp) {
>      } else if (value > max) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     name ? name : "null", type);

Results in an empty if which looks funny.  This one could be a manual
touchup later.

> @@ -211,12 +200,10 @@ static void visit_type_intN(Visitor *v, int64_t *obj, 
> const char *name,
>                              int64_t min, int64_t max, const char *type,
>                              Error **errp)
>  {
> -    Error *err = NULL;
>      int64_t value = *obj;
>  
> -    v->type_int64(v, name, &value, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    v->type_int64(v, name, &value, errp);
> +    if (*errp) {
>      } else if (value < min || value > max) {

and again

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel