Thank you for the review I applied this to master.
On Wed, Jan 31, 2018 at 02:57:01PM -0800, Yifeng Sun wrote: > 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
