[ovs-dev] [PATCH] util: Document and rely on ovs_assert() always evaluating its argument.
Ben Pfaff
blp at ovn.org
Wed Jan 31 19:23:24 UTC 2018
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
More information about the dev
mailing list