[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