Thanks, looks good to me. Reviewed-by: Yifeng Sun <[email protected]>
On Wed, Jan 31, 2018 at 11:23 AM, Ben Pfaff <[email protected]> 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 <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
