diff mbox series

[ovs-dev,v2] ofproto: Fix mod flow table name not to take effect.

Message ID 20240403071136.45174-1-zhouyuhao.philozhou@bytedance.com
State Under Review
Delegated to: Simon Horman
Headers show
Series [ovs-dev,v2] ofproto: Fix mod flow table name not to take effect. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Yuhao zhou April 3, 2024, 7:11 a.m. UTC 
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>
---
V1 -> V2:
  1. Add oftable_may_set_name name len check.
  2. Use OFP_MAX_TABLE_NAME_LEN as length of strncmp.

Signed-off-by: zhouyuhao.philozhou <zhouyuhao.philozhou@bytedance.com>
---
 ofproto/ofproto.c |  9 ++++-----
 tests/ofproto.at  | 12 ++++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

Comments

Simon Horman April 3, 2024, 10:10 a.m. UTC | #1 
On Wed, Apr 03, 2024 at 03:11:36PM +0800, 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".
> 

Hi Yuhao Zhou,

Thanks for your patch.

I did have a little trouble parsing the patch description, so could I
suggest updating it to something like this the following:

Also, as a fix a Fixes tag seems appropriate.
After looking over the code and git history I've made a suggestion below.

Subject: [PATCH v3] ofproto: Allow rename of table to prefix of current name.

Ensure that a flow table rename request takes place even in the case
where the new name matches a prefix, but not all, of the existing name.

Currently this is not the case as checks do not take into account
that the old name may be longer than the new name.

F.e.
    table 10: "good"
    There will be no change if mod the table's name with "g" "go" "goo".

Fixes: 254750ceb233 ("Add support for limiting the number of flows in an OpenFlow flow table.")

> Signed-off-by: zhouyuhao.philozhou <zhouyuhao.philozhou@bytedance.com>
> ---
> V1 -> V2:
>   1. Add oftable_may_set_name name len check.
>   2. Use OFP_MAX_TABLE_NAME_LEN as length of strncmp.
> 
> Signed-off-by: zhouyuhao.philozhou <zhouyuhao.philozhou@bytedance.com>
> ---
>  ofproto/ofproto.c |  9 ++++-----
>  tests/ofproto.at  | 12 ++++++++++++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..f6facae56 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -9289,11 +9289,12 @@ oftable_destroy(struct oftable *table)
>  static void
>  oftable_set_name(struct oftable *table, const char *name, int level)
>  {
> -    int len = name ? strnlen(name, OFP_MAX_TABLE_NAME_LEN) : 0;
>      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,
> +                                            OFP_MAX_TABLE_NAME_LEN)) {
> +                    int len = strnlen(name, OFP_MAX_TABLE_NAME_LEN);
>                      free(table->name);
>                      table->name = xmemdup0(name, len);
>                  }
> @@ -9318,10 +9319,8 @@ oftable_may_set_name(const struct oftable *table, const char *name, int level)
>      return (level >= table->name_level
>              || !name
>              || !table->name
> -            || !strncmp(name, table->name,
> -                        strnlen(name, OFP_MAX_TABLE_NAME_LEN)));
> +            || !strncmp(name, table->name,OFP_MAX_TABLE_NAME_LEN));

Is it possible to ad a test-case for the oftable_may_set_name() change?
I think the test added by this patch only exercises the oftable_set_name()
change.

>  }
> -

nit: please don't delete this blank line.

>  /* oftables support a choice of two policies when adding a rule would cause the
>   * number of flows in the table to exceed the configured maximum number: either
>   * they can refuse to add the new flow or they can evict some existing flow.
> 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.

Here, could I suggest the following?

# Make sure that table name changes take effect when the new name is
# a prefix of the old name.

> +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

...
diff mbox series

Patch

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 122a06f30..f6facae56 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -9289,11 +9289,12 @@  oftable_destroy(struct oftable *table)
 static void
 oftable_set_name(struct oftable *table, const char *name, int level)
 {
-    int len = name ? strnlen(name, OFP_MAX_TABLE_NAME_LEN) : 0;
     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,
+                                            OFP_MAX_TABLE_NAME_LEN)) {
+                    int len = strnlen(name, OFP_MAX_TABLE_NAME_LEN);
                     free(table->name);
                     table->name = xmemdup0(name, len);
                 }
@@ -9318,10 +9319,8 @@  oftable_may_set_name(const struct oftable *table, const char *name, int level)
     return (level >= table->name_level
             || !name
             || !table->name
-            || !strncmp(name, table->name,
-                        strnlen(name, OFP_MAX_TABLE_NAME_LEN)));
+            || !strncmp(name, table->name,OFP_MAX_TABLE_NAME_LEN));
 }
-
 /* oftables support a choice of two policies when adding a rule would cause the
  * number of flows in the table to exceed the configured maximum number: either
  * they can refuse to add the new flow or they can evict some existing flow.
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