diff mbox series

[ovs-dev] odp-util: Fix NSH mask parsing.

Message ID 20190715135834.30963-1-i.maximets@samsung.com
State Accepted
Headers show
Series [ovs-dev] odp-util: Fix NSH mask parsing. | expand

Commit Message

Ilya Maximets July 15, 2019, 1:58 p.m. UTC 
'odp_flow_key_to_flow__' is used to parse both keys and masks.
However, 'odp_nsh_key_from_attr' expects only 'key' as an argument
and fails to parse masks with ODP_FIT_ERROR which causes userspace
system tests failures:

 # make check-system-userspace TESTSUITEFLAGS='-k nsh'

 |odp_util(revalidator)|WARN|
 OVS_NSH_KEY_ATTR_MD1 present but declared mdtype 0 is not 1 (NSH_M_TYPE1)

 |odp_util(revalidator)|WARN|
 the flow mask in error is:
     <...> nsh(flags=0ttl=0,mdtype=0,np=255,spi=0x0,si=0),
 for the following flow key:
     <...> nsh_ttl=8,nsh_mdtype=1,nsh_np=3,nsh_spi=0x64,nsh_si=3,
     nsh_c1=0x1020304,nsh_c2=0x5060708,nsh_c3=0x90a0b0c,nsh_c4=0xd0e0f10
     <...>

Fix that by passing the additional argument 'is_mask' to make it be
like all other parsing functions.

Additionally fixed missing comma in the 'format_nsh_key'.

CC: Yi Yang <yi.y.yang@intel.com>
Fixes: f59cb331c481 ("nsh: rework NSH netlink keys and actions")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/odp-util.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

William Tu July 15, 2019, 5:41 p.m. UTC | #1 
On Mon, Jul 15, 2019 at 6:58 AM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> 'odp_flow_key_to_flow__' is used to parse both keys and masks.
> However, 'odp_nsh_key_from_attr' expects only 'key' as an argument
> and fails to parse masks with ODP_FIT_ERROR which causes userspace
> system tests failures:
>
>  # make check-system-userspace TESTSUITEFLAGS='-k nsh'
>
>  |odp_util(revalidator)|WARN|
>  OVS_NSH_KEY_ATTR_MD1 present but declared mdtype 0 is not 1 (NSH_M_TYPE1)
>
>  |odp_util(revalidator)|WARN|
>  the flow mask in error is:
>      <...> nsh(flags=0ttl=0,mdtype=0,np=255,spi=0x0,si=0),
>  for the following flow key:
>      <...> nsh_ttl=8,nsh_mdtype=1,nsh_np=3,nsh_spi=0x64,nsh_si=3,
>      nsh_c1=0x1020304,nsh_c2=0x5060708,nsh_c3=0x90a0b0c,nsh_c4=0xd0e0f10
>      <...>
>
> Fix that by passing the additional argument 'is_mask' to make it be
> like all other parsing functions.
>
> Additionally fixed missing comma in the 'format_nsh_key'.
>
> CC: Yi Yang <yi.y.yang@intel.com>
> Fixes: f59cb331c481 ("nsh: rework NSH netlink keys and actions")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---

LGTM, thanks for the fix.
Acked-by: William Tu <u9012063@gmail.com>

>  lib/odp-util.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
Ilya Maximets July 17, 2019, 8:09 a.m. UTC | #2 
On 15.07.2019 20:41, William Tu wrote:
> On Mon, Jul 15, 2019 at 6:58 AM Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> 'odp_flow_key_to_flow__' is used to parse both keys and masks.
>> However, 'odp_nsh_key_from_attr' expects only 'key' as an argument
>> and fails to parse masks with ODP_FIT_ERROR which causes userspace
>> system tests failures:
>>
>>  # make check-system-userspace TESTSUITEFLAGS='-k nsh'
>>
>>  |odp_util(revalidator)|WARN|
>>  OVS_NSH_KEY_ATTR_MD1 present but declared mdtype 0 is not 1 (NSH_M_TYPE1)
>>
>>  |odp_util(revalidator)|WARN|
>>  the flow mask in error is:
>>      <...> nsh(flags=0ttl=0,mdtype=0,np=255,spi=0x0,si=0),
>>  for the following flow key:
>>      <...> nsh_ttl=8,nsh_mdtype=1,nsh_np=3,nsh_spi=0x64,nsh_si=3,
>>      nsh_c1=0x1020304,nsh_c2=0x5060708,nsh_c3=0x90a0b0c,nsh_c4=0xd0e0f10
>>      <...>
>>
>> Fix that by passing the additional argument 'is_mask' to make it be
>> like all other parsing functions.
>>
>> Additionally fixed missing comma in the 'format_nsh_key'.
>>
>> CC: Yi Yang <yi.y.yang@intel.com>
>> Fixes: f59cb331c481 ("nsh: rework NSH netlink keys and actions")
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
> 
> LGTM, thanks for the fix.
> Acked-by: William Tu <u9012063@gmail.com>

Thanks! Applied to master.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 3a28877e3..84ea4c148 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -264,7 +264,7 @@  static void
 format_nsh_key(struct ds *ds, const struct ovs_key_nsh *key)
 {
     ds_put_format(ds, "flags=%d", key->flags);
-    ds_put_format(ds, "ttl=%d", key->ttl);
+    ds_put_format(ds, ",ttl=%d", key->ttl);
     ds_put_format(ds, ",mdtype=%d", key->mdtype);
     ds_put_format(ds, ",np=%d", key->np);
     ds_put_format(ds, ",spi=0x%x",
@@ -2798,11 +2798,14 @@  odp_parse_error(struct vlog_rate_limit *rl, char **errorp,
 }
 
 /* Parses OVS_KEY_ATTR_NSH attribute 'attr' into 'nsh' and 'nsh_mask' and
- * returns fitness.  If 'errorp' is nonnull and the function returns
- * ODP_FIT_ERROR, stores a malloc()'d error message in '*errorp'. */
-enum odp_key_fitness
-odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
-                      struct ovs_key_nsh *nsh_mask, char **errorp)
+ * returns fitness.  If the attribute is a key, 'is_mask' should be false;
+ * if it is a mask, 'is_mask' should be true.  If 'errorp' is nonnull and the
+ * function returns ODP_FIT_ERROR, stores a malloc()'d error message in
+ * '*errorp'. */
+static enum odp_key_fitness
+odp_nsh_key_from_attr__(const struct nlattr *attr, bool is_mask,
+                        struct ovs_key_nsh *nsh, struct ovs_key_nsh *nsh_mask,
+                        char **errorp)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     if (errorp) {
@@ -2877,7 +2880,7 @@  odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
         return ODP_FIT_TOO_MUCH;
     }
 
-    if (has_md1 && nsh->mdtype != NSH_M_TYPE1 && !nsh_mask) {
+    if (!is_mask && has_md1 && nsh->mdtype != NSH_M_TYPE1 && !nsh_mask) {
         odp_parse_error(&rl, errorp, "OVS_NSH_KEY_ATTR_MD1 present but "
                         "declared mdtype %"PRIu8" is not %d (NSH_M_TYPE1)",
                         nsh->mdtype, NSH_M_TYPE1);
@@ -2887,6 +2890,17 @@  odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
     return ODP_FIT_PERFECT;
 }
 
+/* Parses OVS_KEY_ATTR_NSH attribute 'attr' into 'nsh' and 'nsh_mask' and
+ * returns fitness.  The attribute should be a key (not a mask).  If 'errorp'
+ * is nonnull and the function returns ODP_FIT_ERROR, stores a malloc()'d error
+ * message in '*errorp'. */
+enum odp_key_fitness
+odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
+                      struct ovs_key_nsh *nsh_mask, char **errorp)
+{
+    return odp_nsh_key_from_attr__(attr, false, nsh, nsh_mask, errorp);
+}
+
 /* Parses OVS_KEY_ATTR_TUNNEL attribute 'attr' into 'tun' and returns fitness.
  * If the attribute is a key, 'is_mask' should be false; if it is a mask,
  * 'is_mask' should be true.  If 'errorp' is nonnull and the function returns
@@ -6750,8 +6764,9 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
             *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_NSH;
         }
         if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_NSH)) {
-            if (odp_nsh_key_from_attr(attrs[OVS_KEY_ATTR_NSH], &flow->nsh,
-                                      NULL, errorp) == ODP_FIT_ERROR) {
+            if (odp_nsh_key_from_attr__(attrs[OVS_KEY_ATTR_NSH],
+                                        is_mask, &flow->nsh,
+                                        NULL, errorp) == ODP_FIT_ERROR) {
                 return ODP_FIT_ERROR;
             }
             if (is_mask) {