Thanks for the review. I fixed the obvious stuff and just dropped the
request.
On Fri, Jan 11, 2019 at 01:07:36AM -0800, Justin Pettit wrote:
>
> > On Aug 30, 2018, at 1:00 PM, Ben Pfaff <b...@ovn.org> wrote:
> > @@ -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?
I think so. I broke this out as a separate patch and backported it.
> > @@ -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.
I think this doesn't need to be there at all. It was more of a debug
print during development. I dropped the change.
> > @@ -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?
None of the other cases considers OFP16_VERSION, so it's consistent with
those. It's probably a bug overall though.
OF1.6 is dead and will never be released. It probably makes sense to
just remove all support. I'll work on that.
> > +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?
Good catch, thanks.
I moved the existing free call to just before checking the return value:
@@ -3995,6 +3994,7 @@ handle_table_features_request(struct ofconn *ofconn,
query_tables(ofproto, &features, NULL);
if (!ovs_list_is_singleton(msgs) || msg->size) {
int retval = handle_table_features_change(ofconn, msgs, features);
+ free(features);
if (retval) {
if (retval < 0) {
/* handle_table_features_change() already sent an error. */
@@ -4005,7 +4005,6 @@ handle_table_features_request(struct ofconn *ofconn,
}
/* Features may have changed, re-query. */
- free(features);
query_tables(ofproto, &features, NULL);
}
> >
> > +/* 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.
It appears that if table->name is NULL, then table->name_level is always
0, so strncmp() doesn't get called.
I added || !table->name to the condition just in case. It would suck to
segfault because of a bug in table names.
> The strncmp() check seems to return false if the name is being changed, which
> seems like different behavior from oftable_may_set_name().
If this check gets to the point of calling strncmp(), then the name
can't be changed because the level is too low, but we are still willing
to return true if the table's current name is the same as the requested
one, hence the !strncmp() check.
> > 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"?
OK, done.
> Also, it may be worth documenting that the name must be cleared before
> it can be changed.
It's not necessary to do that, unless it's already set to something
different via OVSDB. The parenthetical tries to document that.
> > 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?
Might as well.
> Acked-by: Justin Pettit <jpet...@ovn.org>
Thanks for the review!
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
