> On Aug 30, 2018, at 1:00 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> diff --git a/lib/ofp-table.c b/lib/ofp-table.c
> index afa9b9103b88..e295441af20a 100644
> --- a/lib/ofp-table.c
> +++ b/lib/ofp-table.c
> @@ -350,11 +350,15 @@ parse_oxms(struct ofpbuf *payload, bool loose,
> /* Converts an OFPMP_TABLE_FEATURES request or reply in 'msg' into an abstract
> * ofputil_table_features in 'tf'.
> *
> - * If 'loose' is true, this function ignores properties and values that it
> does
> - * not understand, as a controller would want to do when interpreting
> - * capabilities provided by a switch. If 'loose' is false, this function
> - * treats unknown properties and values as an error, as a switch would want
> to
> - * do when interpreting a configuration request made by a controller.
> + * If 'raw_properties' is nonnull function ignores properties and values that
I think it would be clearer if "the" or "this" was before "function".
> @@ -418,8 +425,11 @@ ofputil_decode_table_features(struct ofpbuf *msg,
>
> struct ofpbuf properties = ofpbuf_const_initializer(ofpbuf_pull(msg, len),
> len);
> - tf->any_properties = properties.size > 0;
> ofpbuf_pull(&properties, sizeof *otf);
> + tf->any_properties = properties.size > 0;
Is changing the order of these operations a bug fix that should be backported?
> @@ -514,7 +524,7 @@ ofputil_decode_table_features(struct ofpbuf *msg,
> * OpenFlow 1.5 requires all of them if any property is present. */
> if ((seen & OFPTFPT13_REQUIRED) != OFPTFPT13_REQUIRED
> && (tf->any_properties || oh->version < OFP15_VERSION)) {
> - VLOG_WARN_RL(&rl, "table features message missing required
> property");
> + VLOG_WARN_RL(&rl, "table features message missing required property
> %d", seen);
Since 'seen' is a bitmap, it might be easer to read in hex.
> @@ -1293,6 +1314,10 @@ parse_ofp_table_mod(struct ofputil_table_mod *tm,
> const char *table_id,
> } else if (!strcmp(setting, "novacancy")) {
> tm->vacancy = OFPUTIL_TABLE_VACANCY_OFF;
> *usable_versions = (1 << OFP14_VERSION) | (1u << OFP15_VERSION);
> + } else if (tm->table_id != OFPTT_ALL && !strncmp(setting, "name:", 5)) {
> + *namep = setting + 5;
> + *usable_versions = ((1 << OFP13_VERSION) | (1u << OFP14_VERSION)
> + | (1u << OFP15_VERSION));
Is OFP16_VERSION left out on a purpose?
> +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in
> 'sub',
> + * false otherwise. 'super' and 'sub' both have 'n_bits' bits. */
> +static bool
> +bitmap_is_superset(const unsigned long int *super,
> + const unsigned long int *sub, size_t n_bits)
> +{
> + size_t n_longs = bitmap_n_longs(n_bits);
> + for (size_t i = 0; i < n_longs; i++) {
> + if (!ulong_is_superset(super[i], sub[i])) {
> + return false;
> + }
> + }
> + return true;
> +}
Do you think it's worth putting this in "bitmap.h"?
> +/* Returns true if the 1-bits in 'super' are a superset of the 1-bits in
> 'sub',
> + * false otherwise. */
> +static bool
> +mf_bitmap_is_superset(const struct mf_bitmap *super,
> + const struct mf_bitmap *sub)
> +{
> + return bitmap_is_superset(super->bm, sub->bm, MFF_N_IDS);
> +}
Do you think it's worth putting this in "meta-flow.h"?
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index c9d73c10c0ae..d65a3fea1559 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
>
> +static void
> +copy_properties(struct ofputil_table_features *dst,
> + const struct ofputil_table_features *src)
> +{
> + dst->any_properties = src->any_properties;
> + if (src->any_properties) {
> + dst->nonmiss = src->nonmiss;
> + dst->miss = src->miss;
> + dst->match = src->match;
> + dst->mask = src->mask;
> + dst->wildcard = src->wildcard;
> + }
> +
> +}
I think there's an empty newline above.
> +static void
> +handle_table_features_request(struct ofconn *ofconn,
> + const struct ovs_list *msgs)
> +{
> ...
> + /* Update the table features configuration, if requested. */
> struct ofputil_table_features *features;
> query_tables(ofproto, &features, NULL);
> + if (!ovs_list_is_singleton(msgs) || msg->size) {
> + int retval = handle_table_features_change(ofconn, msgs, features);
> + if (retval) {
> + if (retval < 0) {
> + /* handle_table_features_change() already sent an error. */
> + } else {
> + ofconn_send_error(ofconn, request, retval);
> + }
> + return;
Does 'features' need to be freed?
>
> +/* Returns true if oftable_set_name(table, name, level) would be effective,
> + * false otherwise. */
> +static bool
> +oftable_may_set_name(const struct oftable *table, const char *name, int
> level)
> +{
> + return (level >= table->name_level
> + || !name
> + || !strncmp(name, table->name,
> + strnlen(name, OFP_MAX_TABLE_NAME_LEN)));
I think 'table->name' can be NULL, in which case, I believe the behavior of
strncmp is undefined.
The strncmp() check seems to return false if the name is being changed, which
seems like different behavior from oftable_may_set_name().
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 4850f4a25504..968e1ec347af 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -88,6 +88,17 @@ Send to controller. (This is how an OpenFlow 1.0 switch
> always
> handles packets that do not match any flow in the last table.)
> .RE
> .IP
> +In OpenFlow 1.3 and later (which must be enabled with the \fB\-O\fR
> +option) and Open vSwitch 2.11 and later only, \fBmod\-table\fR can
> +change the name of a table:
> +.RS
> +.IP \fBname:\fInew-name\fR
> +Changes the name of the table to \fInew-name\fR. (This will be
> +ineffective if the name is set via the \fBname\fR column in the
> +\fBFlow_Table\fR table in the \fBOpen_vSwitch\fR database as described
> +in \fBovs\-vswitchd.conf.db\fR(5).)
Do you think it's worth mentioning that the name can be cleared by leaving off
"new-name"? Also, it may be worth documenting that the name must be cleared
before it can be changed.
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index aa9a1291e60a..82b2b42a92cd 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -2607,27 +2687,32 @@ ofctl_mod_table(struct ovs_cmdl_context *ctx)
>
> + if (name) {
> + change_table_name(vconn, tm.table_id, name);
> + } else {
> + /* For OpenFlow 1.4+, ovs-ofctl mod-table should not affect
> + * table-config properties that the user didn't ask to change, so it
> is
> + * necessary to restore the current configuration of table-config
> + * parameters using OFPMP14_TABLE_DESC request. */
> + if ((allowed_versions & (1u << OFP14_VERSION)) ||
> + (allowed_versions & (1u << OFP15_VERSION))) {
Should OFP16_VERSION be included?
Acked-by: Justin Pettit <jpet...@ovn.org>
--Justin
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
