| Message ID | 20240308083247.46083-1-zhouyuhao.philozhou@bytedance.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofproto: Fix mod flow table name not to take effect. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/8/24 09:32, Yuhao zhou via dev wrote: > From: "zhouyuhao.philozhou" <zhouyuhao.philozhou@bytedance.com> > > When mod a flow table's name with table's prefix name, there > will be no change. Because when check whether the new and old > name are the same, only compare the length of the new name. > > Case: > table 10: "good" > There will be no change if mod the table's name with "g" "go" "goo". > > Signed-off-by: zhouyuhao.philozhou <zhouyuhao.philozhou@bytedance.com> > --- > ofproto/ofproto.c | 4 +++- > tests/ofproto.at | 12 ++++++++++++ > 2 files changed, 15 insertions(+), 1 deletion(-) Good catch! > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 122a06f30..bf7ed91b1 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char *name, int level) > if (level >= table->name_level) { > if (name) { > if (name[0]) { > - if (!table->name || strncmp(name, table->name, len)) { > + if (!table->name > + || strncmp(name, table->name, len) > + || len != strlen(table->name)) { Maybe we can just use OFP_MAX_TABLE_NAME_LEN in the strncmp call? Will it produce the same result? Also, there is an oftable_may_set_name() function just below, it will need the same change. > free(table->name); > table->name = xmemdup0(name, len); > } > diff --git a/tests/ofproto.at b/tests/ofproto.at > index 2889f81fb..09c57b292 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -2523,6 +2523,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], > table 253: > ]) > > +# Make sure that the new name is old table's name prefix can also take effect. s/is old table's name/equal to the old name's/ Best regards, Ilya Maximets.
On 3/16/24 01:44, Ilya Maximets wrote: > On 3/8/24 09:32, Yuhao zhou via dev wrote: >> From: "zhouyuhao.philozhou" <zhouyuhao.philozhou@bytedance.com> >> >> When mod a flow table's name with table's prefix name, there >> will be no change. Because when check whether the new and old >> name are the same, only compare the length of the new name. >> >> Case: >> table 10: "good" >> There will be no change if mod the table's name with "g" "go" "goo". >> >> Signed-off-by: zhouyuhao.philozhou <zhouyuhao.philozhou@bytedance.com> >> --- Also, please, add a version number to the patch subject while sending new versions, e.g. [PATCH v3] ofproto: ... And add a small description on what changed between versions here, after the '---', but before the diff. Best regards, Ilya Maximets. >> ofproto/ofproto.c | 4 +++- >> tests/ofproto.at | 12 ++++++++++++ >> 2 files changed, 15 insertions(+), 1 deletion(-) > > Good catch! > >> >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 122a06f30..bf7ed91b1 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char *name, int level) >> if (level >= table->name_level) { >> if (name) { >> if (name[0]) { >> - if (!table->name || strncmp(name, table->name, len)) { >> + if (!table->name >> + || strncmp(name, table->name, len) >> + || len != strlen(table->name)) { > > Maybe we can just use OFP_MAX_TABLE_NAME_LEN in the strncmp call? > Will it produce the same result? > > Also, there is an oftable_may_set_name() function just below, it > will need the same change. > >> free(table->name); >> table->name = xmemdup0(name, len); >> } >> diff --git a/tests/ofproto.at b/tests/ofproto.at >> index 2889f81fb..09c57b292 100644 >> --- a/tests/ofproto.at >> +++ b/tests/ofproto.at >> @@ -2523,6 +2523,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], >> table 253: >> ]) >> >> +# Make sure that the new name is old table's name prefix can also take effect. > > s/is old table's name/equal to the old name's/ > > Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 122a06f30..bf7ed91b1 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char *name, int level) if (level >= table->name_level) { if (name) { if (name[0]) { - if (!table->name || strncmp(name, table->name, len)) { + if (!table->name + || strncmp(name, table->name, len) + || len != strlen(table->name)) { free(table->name); table->name = xmemdup0(name, len); } diff --git a/tests/ofproto.at b/tests/ofproto.at index 2889f81fb..09c57b292 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -2523,6 +2523,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], table 253: ]) +# Make sure that the new name is old table's name prefix can also take effect. +AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 3 name:thr]) +AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'], + [0], [dnl + table 0 ("zero"): + table 1 ("one"): ditto + table 2: ditto + table 3 ("thr"): ditto + tables 4...252: ditto + table 253: +]) + OVS_VSWITCHD_STOP AT_CLEANUP