[ovs-dev] [PATCH] util: Document and rely on ovs_assert() always evaluating its argument.

Yifeng Sun pkusunyifeng at gmail.com
Wed Jan 31 22:57:01 UTC 2018


Thanks, looks good to me.

Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>

On Wed, Jan 31, 2018 at 11:23 AM, Ben Pfaff <blp at ovn.org> wrote:

> The ovs_assert() macro always evaluates its argument, even when NDEBUG is
> defined so that failure is ignored.  This behavior wasn't documented, and
> thus a lot of code didn't rely on it.  This commit documents the behavior
> and simplifies bits of code that heretofore didn't rely on it.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  include/openvswitch/util.h   |  7 +++++--
>  lib/cmap.c                   |  6 ++----
>  lib/conntrack.c              |  6 ++----
>  lib/hmapx.c                  |  6 ++----
>  lib/odp-execute.c            |  5 +----
>  lib/odp-util.c               |  5 +----
>  lib/ofp-msgs.c               | 32 ++++++++------------------------
>  lib/ofp-util.c               | 11 ++++-------
>  lib/ovsdb-data.c             |  4 +---
>  lib/shash.c                  |  3 +--
>  lib/sset.c                   |  6 ++----
>  ofproto/ofproto-dpif-xlate.c |  7 +++----
>  ovsdb/ovsdb-server.c         |  4 +---
>  ovsdb/replication.c          |  3 +--
>  14 files changed, 34 insertions(+), 71 deletions(-)
>
> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
> index ad1b184f78d9..b4d49ee21804 100644
> --- a/include/openvswitch/util.h
> +++ b/include/openvswitch/util.h
> @@ -44,8 +44,11 @@ const char *ovs_get_program_version(void);
>       : (X) <= UINT_MAX / (Y) ? (unsigned int) (X) * (unsigned int) (Y)  \
>       : UINT_MAX)
>
> -/* Like the standard assert macro, except writes the failure message to
> the
> - * log. */
> +/* Like the standard assert macro, except:
> + *
> + *    - Writes the failure message to the log.
> + *
> + *    - Always evaluates the condition, even with NDEBUG. */
>  #ifndef NDEBUG
>  #define ovs_assert(CONDITION)                                           \
>      (OVS_LIKELY(CONDITION)                                              \
> diff --git a/lib/cmap.c b/lib/cmap.c
> index af29639b049c..07719a8fd286 100644
> --- a/lib/cmap.c
> +++ b/lib/cmap.c
> @@ -843,11 +843,9 @@ cmap_replace(struct cmap *cmap, struct cmap_node
> *old_node,
>      struct cmap_impl *impl = cmap_get_impl(cmap);
>      uint32_t h1 = rehash(impl, hash);
>      uint32_t h2 = other_hash(h1);
> -    bool ok;
>
> -    ok = cmap_replace__(impl, old_node, new_node, hash, h1)
> -        || cmap_replace__(impl, old_node, new_node, hash, h2);
> -    ovs_assert(ok);
> +    ovs_assert(cmap_replace__(impl, old_node, new_node, hash, h1) ||
> +               cmap_replace__(impl, old_node, new_node, hash, h2));
>
>      if (!new_node) {
>          impl->n--;
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 562e767833d1..fe5fd0fe8be1 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -3080,11 +3080,9 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct
> ct_addr v6_addr_rep,
>          return 0;
>      }
>
> -    const char *rc;
>      char v6_addr_str[IPV6_SCAN_LEN] = {0};
> -    rc = inet_ntop(AF_INET6, &v6_addr_rep.ipv6_aligned, v6_addr_str,
> -              IPV6_SCAN_LEN - 1);
> -    ovs_assert(rc != NULL);
> +    ovs_assert(inet_ntop(AF_INET6, &v6_addr_rep.ipv6_aligned, v6_addr_str,
> +                         IPV6_SCAN_LEN - 1));
>
>      size_t replace_addr_size = strlen(v6_addr_str);
>
> diff --git a/lib/hmapx.c b/lib/hmapx.c
> index 0440767c9c97..eadfe640ac11 100644
> --- a/lib/hmapx.c
> +++ b/lib/hmapx.c
> @@ -116,8 +116,7 @@ hmapx_add(struct hmapx *map, void *data)
>  void
>  hmapx_add_assert(struct hmapx *map, void *data)
>  {
> -    bool added OVS_UNUSED = hmapx_add(map, data);
> -    ovs_assert(added);
> +    ovs_assert(hmapx_add(map, data));
>  }
>
>  /* Removes all of the nodes from 'map'. */
> @@ -156,8 +155,7 @@ hmapx_find_and_delete(struct hmapx *map, const void
> *data)
>  void
>  hmapx_find_and_delete_assert(struct hmapx *map, const void *data)
>  {
> -    bool deleted OVS_UNUSED = hmapx_find_and_delete(map, data);
> -    ovs_assert(deleted);
> +    ovs_assert(hmapx_find_and_delete(map, data));
>  }
>
>  /* Searches for 'data' in 'map'.  Returns its node, if found, otherwise a
> null
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 20f33eb57acb..12bba83ea32c 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -198,10 +198,7 @@ odp_set_sctp(struct dp_packet *packet, const struct
> ovs_key_sctp *key,
>  static void
>  odp_set_tunnel_action(const struct nlattr *a, struct flow_tnl *tun_key)
>  {
> -    enum odp_key_fitness fitness;
> -
> -    fitness = odp_tun_key_from_attr(a, tun_key);
> -    ovs_assert(fitness != ODP_FIT_ERROR);
> +    ovs_assert(odp_tun_key_from_attr(a, tun_key) != ODP_FIT_ERROR);
>  }
>
>  static void
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 6a29a76de5cd..14d5af097ee4 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -6847,8 +6847,6 @@ commit_mpls_action(const struct flow *flow, struct
> flow *base,
>              /* Otherwise, if there more LSEs in base than are common
> between
>               * base and flow then pop the topmost one. */
>              ovs_be16 dl_type;
> -            bool popped;
> -
>              /* If all the LSEs are to be popped and this is not the
> outermost
>               * LSE then use ETH_TYPE_MPLS as the ethertype parameter of
> the
>               * POP_MPLS action instead of flow->dl_type.
> @@ -6865,8 +6863,7 @@ commit_mpls_action(const struct flow *flow, struct
> flow *base,
>                  dl_type = flow->dl_type;
>              }
>              nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_POP_MPLS,
> dl_type);
> -            popped = flow_pop_mpls(base, base_n, flow->dl_type, NULL);
> -            ovs_assert(popped);
> +            ovs_assert(flow_pop_mpls(base, base_n, flow->dl_type, NULL));
>              base_n--;
>          }
>      }
> diff --git a/lib/ofp-msgs.c b/lib/ofp-msgs.c
> index f9660fc81034..dd8894a524eb 100644
> --- a/lib/ofp-msgs.c
> +++ b/lib/ofp-msgs.c
> @@ -313,8 +313,7 @@ static void
>  ofphdrs_decode_assert(struct ofphdrs *hdrs,
>                        const struct ofp_header *oh, size_t length)
>  {
> -    enum ofperr error = ofphdrs_decode(hdrs, oh, length);
> -    ovs_assert(!error);
> +    ovs_assert(!ofphdrs_decode(hdrs, oh, length));
>  }
>
>  static bool
> @@ -424,11 +423,8 @@ ofpraw_decode(enum ofpraw *raw, const struct
> ofp_header *oh)
>  enum ofpraw
>  ofpraw_decode_assert(const struct ofp_header *oh)
>  {
> -    enum ofperr error;
>      enum ofpraw raw;
> -
> -    error = ofpraw_decode(&raw, oh);
> -    ovs_assert(!error);
> +    ovs_assert(!ofpraw_decode(&raw, oh));
>      return raw;
>  }
>
> @@ -524,11 +520,8 @@ ofpraw_pull(enum ofpraw *rawp, struct ofpbuf *msg)
>  enum ofpraw
>  ofpraw_pull_assert(struct ofpbuf *msg)
>  {
> -    enum ofperr error;
>      enum ofpraw raw;
> -
> -    error = ofpraw_pull(&raw, msg);
> -    ovs_assert(!error);
> +    ovs_assert(!ofpraw_pull(&raw, msg));
>      return raw;
>  }
>
> @@ -633,11 +626,9 @@ ofpraw_alloc_stats_reply(const struct ofp_header
> *request,
>  {
>      enum ofpraw request_raw;
>      enum ofpraw reply_raw;
> -    enum ofperr error;
>
> -    error = ofpraw_decode_partial(&request_raw, request,
> -                                  ntohs(request->length));
> -    ovs_assert(!error);
> +    ovs_assert(!ofpraw_decode_partial(&request_raw, request,
> +                                      ntohs(request->length)));
>
>      reply_raw = ofpraw_stats_request_to_reply(request_raw,
> request->version);
>      ovs_assert(reply_raw);
> @@ -703,11 +694,9 @@ ofpraw_put_reply(enum ofpraw raw, const struct
> ofp_header *request,
>  void
>  ofpraw_put_stats_reply(const struct ofp_header *request, struct ofpbuf
> *buf)
>  {
> -    enum ofperr error;
>      enum ofpraw raw;
>
> -    error = ofpraw_decode_partial(&raw, request, ntohs(request->length));
> -    ovs_assert(!error);
> +    ovs_assert(!ofpraw_decode_partial(&raw, request,
> ntohs(request->length)));
>
>      raw = ofpraw_stats_request_to_reply(raw, request->version);
>      ovs_assert(raw);
> @@ -801,7 +790,6 @@ ofpraw_stats_request_to_reply(enum ofpraw raw,
> uint8_t version)
>      const struct raw_instance *instance = raw_instance_get(info, version);
>      enum ofpraw reply_raw;
>      struct ofphdrs hdrs;
> -    enum ofperr error;
>
>      hdrs = instance->hdrs;
>      switch ((enum ofp_version)hdrs.version) {
> @@ -822,8 +810,7 @@ ofpraw_stats_request_to_reply(enum ofpraw raw,
> uint8_t version)
>          OVS_NOT_REACHED();
>      }
>
> -    error = ofpraw_from_ofphdrs(&reply_raw, &hdrs);
> -    ovs_assert(!error);
> +    ovs_assert(!ofpraw_from_ofphdrs(&reply_raw, &hdrs));
>
>      return reply_raw;
>  }
> @@ -1016,11 +1003,8 @@ enum ofpraw
>  ofpmp_decode_raw(struct ovs_list *replies)
>  {
>      struct ofpbuf *msg = ofpbuf_from_list(ovs_list_back(replies));
> -    enum ofperr error;
>      enum ofpraw raw;
> -
> -    error = ofpraw_decode_partial(&raw, msg->data, msg->size);
> -    ovs_assert(!error);
> +    ovs_assert(!ofpraw_decode_partial(&raw, msg->data, msg->size));
>      return raw;
>  }
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 597112e4f84e..ed972f835cf9 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -2453,12 +2453,11 @@ ofputil_start_queue_get_config_reply(const struct
> ofp_header *request,
>                                       struct ovs_list *replies)
>  {
>      struct ofpbuf *reply;
> -    enum ofperr error;
>      ofp_port_t port;
>      uint32_t queue;
>
> -    error = ofputil_decode_queue_get_config_request(request, &port,
> &queue);
> -    ovs_assert(!error);
> +    ovs_assert(!ofputil_decode_queue_get_config_request(request, &port,
> +                                                        &queue));
>
>      enum ofpraw raw = ofpraw_decode_assert(request);
>      switch ((int) raw) {
> @@ -6227,8 +6226,7 @@ ofputil_decode_role_status(const struct ofp_header
> *oh,
>                             struct ofputil_role_status *rs)
>  {
>      struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
> -    enum ofpraw raw = ofpraw_pull_assert(&b);
> -    ovs_assert(raw == OFPRAW_OFPT14_ROLE_STATUS);
> +    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_OFPT14_ROLE_STATUS);
>
>      const struct ofp14_role_status *r = b.msg;
>      if (r->role != htonl(OFPCR12_ROLE_NOCHANGE) &&
> @@ -6292,8 +6290,7 @@ ofputil_decode_requestforward(const struct
> ofp_header *outer,
>      struct ofpbuf b = ofpbuf_const_initializer(outer,
> ntohs(outer->length));
>
>      /* Skip past outer message. */
> -    enum ofpraw outer_raw = ofpraw_pull_assert(&b);
> -    ovs_assert(outer_raw == OFPRAW_OFPT14_REQUESTFORWARD);
> +    ovs_assert(ofpraw_pull_assert(&b) == OFPRAW_OFPT14_REQUESTFORWARD);
>
>      /* Validate inner message. */
>      if (b.size < sizeof(struct ofp_header)) {
> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> index 87d8effd1d67..0c9945cd1318 100644
> --- a/lib/ovsdb-data.c
> +++ b/lib/ovsdb-data.c
> @@ -1948,10 +1948,8 @@ ovsdb_datum_union(struct ovsdb_datum *a, const
> struct ovsdb_datum *b,
>          }
>      }
>      if (n != a->n) {
> -        struct ovsdb_error *error;
>          a->n = n;
> -        error = ovsdb_datum_sort(a, type->key.type);
> -        ovs_assert(!error);
> +        ovs_assert(!ovsdb_datum_sort(a, type->key.type));
>      }
>  }
>
> diff --git a/lib/shash.c b/lib/shash.c
> index 3e94b172a65f..318a30ffc0f0 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -143,8 +143,7 @@ shash_add_once(struct shash *sh, const char *name,
> const void *data)
>  void
>  shash_add_assert(struct shash *sh, const char *name, const void *data)
>  {
> -    bool added OVS_UNUSED = shash_add_once(sh, name, data);
> -    ovs_assert(added);
> +    ovs_assert(shash_add_once(sh, name, data));
>  }
>
>  /* Searches for 'name' in 'sh'.  If it does not already exist, adds it
> along
> diff --git a/lib/sset.c b/lib/sset.c
> index be29cc71ef2e..3deb1f9de9be 100644
> --- a/lib/sset.c
> +++ b/lib/sset.c
> @@ -163,8 +163,7 @@ sset_add_and_free(struct sset *set, char *name)
>  void
>  sset_add_assert(struct sset *set, const char *name)
>  {
> -    bool added OVS_UNUSED = sset_add(set, name);
> -    ovs_assert(added);
> +    ovs_assert(sset_add(set, name));
>  }
>
>  /* Adds a copy of each of the 'n' names in 'names' to 'set'. */
> @@ -214,8 +213,7 @@ sset_find_and_delete(struct sset *set, const char
> *name)
>  void
>  sset_find_and_delete_assert(struct sset *set, const char *name)
>  {
> -    bool deleted OVS_UNUSED = sset_find_and_delete(set, name);
> -    ovs_assert(deleted);
> +    ovs_assert(sset_find_and_delete(set, name));
>  }
>
>  /* Removes a string from 'set' and returns a copy of it.  The caller must
> free
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index d594ea40f7ee..315c67e71338 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1940,10 +1940,9 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> *xbundle,
>          int snaplen;
>
>          /* Get the details of the mirror represented by the rightmost
> 1-bit. */
> -        bool has_mirror = mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> -                                     &vlans, &dup_mirrors,
> -                                     &out, &snaplen, &out_vlan);
> -        ovs_assert(has_mirror);
> +        ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> +                              &vlans, &dup_mirrors,
> +                              &out, &snaplen, &out_vlan));
>
>
>          /* If this mirror selects on the basis of VLAN, and it does not
> select
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 7f2d19ef568b..3ac7bf472640 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -1291,11 +1291,9 @@ static void
>  remove_db(struct server_config *config, struct shash_node *node)
>  {
>      struct db *db;
> -    bool ok;
>
>      db = node->data;
> -    ok = ovsdb_jsonrpc_server_remove_db(config->jsonrpc, db->db);
> -    ovs_assert(ok);
> +    ovs_assert(ovsdb_jsonrpc_server_remove_db(config->jsonrpc, db->db));
>
>      close_db(db);
>      shash_delete(config->all_dbs, node);
> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
> index bac46c67f409..fc9ce6472725 100644
> --- a/ovsdb/replication.c
> +++ b/ovsdb/replication.c
> @@ -118,10 +118,9 @@ replication_init(const char *sync_from_, const char
> *exclude_tables,
>  {
>      free(sync_from);
>      sync_from = xstrdup(sync_from_);
> -    char *err = set_blacklist_tables(exclude_tables, false);
>      /* Caller should have verified that the 'exclude_tables' is
>       * parseable. An error here is unexpected. */
> -    ovs_assert(!err);
> +    ovs_assert(!set_blacklist_tables(exclude_tables, false));
>
>      replication_dbs_destroy();
>
> --
> 2.15.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list