[ovs-dev,v3] ovn-controller: Fix parsing of OVN tunnel IDs
diff mbox series

Message ID 1558947352-27052-1-git-send-email-dceara@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev,v3] ovn-controller: Fix parsing of OVN tunnel IDs
Related show

Commit Message

Dumitru Ceara May 27, 2019, 8:55 a.m. UTC
Encap tunnel-ids are of the form:
<chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
In physical_run we were checking if a tunnel-id corresponds
to the local chassis-id by searching if the chassis-id string
is included in the tunnel-id (strstr). This can break quite
easily, for example, if the local chassis-id is a substring
of a remote chassis-id. In that case we were wrongfully
skipping the tunnel creation.

To fix that new tunnel-id creation and parsing functions are added in
encaps.[ch]. These functions are now used everywhere where applicable.

Acked-by: Venu Iyer <iyervl@ymail.com>
Reported-at: https://bugzilla.redhat.com/1708131
Reported-by: Haidong Li <haili@redhat.com>
Fixes: b520ca7 ("Support for multiple VTEP in OVN")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---

v3: Rebase
v2: Update commit message
---
 ovn/controller/bfd.c            | 31 ++++++++-------
 ovn/controller/encaps.c         | 87 ++++++++++++++++++++++++++++++++++++++++-
 ovn/controller/encaps.h         |  6 +++
 ovn/controller/ovn-controller.h |  7 ----
 ovn/controller/physical.c       | 51 +++++++++---------------
 ovn/controller/pinctrl.c        |  4 +-
 6 files changed, 130 insertions(+), 56 deletions(-)

Comments

0-day Robot May 27, 2019, 9:55 a.m. UTC | #1
Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
fatal: sha1 information is lacking or useless (ovn/controller/bfd.c).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 ovn-controller: Fix parsing of OVN tunnel IDs
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Dumitru Ceara May 27, 2019, 10:30 a.m. UTC | #2
On Mon, May 27, 2019 at 11:57 AM 0-day Robot <robot@bytheb.org> wrote:
>
> Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> git-am:
> fatal: sha1 information is lacking or useless (ovn/controller/bfd.c).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 ovn-controller: Fix parsing of OVN tunnel IDs
> The copy of the patch that failed is found in:
>    /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --resolved".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
>
> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>
> Thanks,
> 0-day Robot

Hi Aaron,

I'm not sure what the problem is here. Can it be related to what the
bot is doing?

For the v3 version I had updated the master branch and rebased the dev
branch; then I generated the patch from my dev branch with:
git format-patch -M master --subject-prefix="PATCH v3"  -o _patch/

If i apply the patch with "git am" everything seems to work fine:
$ git checkout -b test origin/master
$ git am _patch/0001-ovn-controller-Fix-parsing-of-OVN-tunnel-IDs.patch
Applying: ovn-controller: Fix parsing of OVN tunnel IDs
$ git log --oneline -1
8c559d4 ovn-controller: Fix parsing of OVN tunnel IDs
$ git --version
git version 1.8.3.1

If I use git-pw it also seems to work fine:
$ git checkout -b test2 origin/master
Branch test2 set up to track remote branch master from origin.
Switched to a new branch 'test2'
$ git-pw patch apply 1105697
Starting new HTTPS connection (1): patchwork.ozlabs.org
Starting new HTTPS connection (1): patchwork.ozlabs.org
Applying: ovn-controller: Fix parsing of OVN tunnel IDs
$ git log --oneline -1
aa83849 ovn-controller: Fix parsing of OVN tunnel IDs

Thanks,
Dumitru
Numan Siddique May 27, 2019, 12:44 p.m. UTC | #3
On Mon, May 27, 2019, 4:01 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On Mon, May 27, 2019 at 11:57 AM 0-day Robot <robot@bytheb.org> wrote:
> >
> > Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out
> your patch.
> > Thanks for your contribution.
> >
> > I encountered some error that I wasn't expecting.  See the details below.
> >
> >
> > git-am:
> > fatal: sha1 information is lacking or useless (ovn/controller/bfd.c).
> > Repository lacks necessary blobs to fall back on 3-way merge.
> > Cannot fall back to three-way merge.
> > Patch failed at 0001 ovn-controller: Fix parsing of OVN tunnel IDs
> > The copy of the patch that failed is found in:
> >
> /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch
> > When you have resolved this problem, run "git am --resolved".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> >
> >
> > Please check this out.  If you feel there has been an error, please
> email aconole@bytheb.org
> >
> > Thanks,
> > 0-day Robot
>
> Hi Aaron,
>
> I'm not sure what the problem is here. Can it be related to what the
> bot is doing?
>
> For the v3 version I had updated the master branch and rebased the dev
> branch; then I generated the patch from my dev branch with:
> git format-patch -M master --subject-prefix="PATCH v3"  -o _patch/
>
> If i apply the patch with "git am" everything seems to work fine:
> $ git checkout -b test origin/master
> $ git am _patch/0001-ovn-controller-Fix-parsing-of-OVN-tunnel-IDs.patch
> Applying: ovn-controller: Fix parsing of OVN tunnel IDs
> $ git log --oneline -1
> 8c559d4 ovn-controller: Fix parsing of OVN tunnel IDs
> $ git --version
> git version 1.8.3.1
>

Looks like the bot is trying to apply the patches to the ovn-org/ovn repo.

Aaron is it possible to change the bot to apply the patches to the new repo
only if "ovn" is with in the [*ovn*] ?

Right now we use  -  "[PATCH ..]ovn: ... " for OVN related patches in the
openvswitch repo.

Thanks
Numan


> If I use git-pw it also seems to work fine:
> $ git checkout -b test2 origin/master
> Branch test2 set up to track remote branch master from origin.
> Switched to a new branch 'test2'
> $ git-pw patch apply 1105697
> Starting new HTTPS connection (1): patchwork.ozlabs.org
> Starting new HTTPS connection (1): patchwork.ozlabs.org
> Applying: ovn-controller: Fix parsing of OVN tunnel IDs
> $ git log --oneline -1
> aa83849 ovn-controller: Fix parsing of OVN tunnel IDs
>
> Thanks,
> Dumitru
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Aaron Conole May 28, 2019, 12:23 p.m. UTC | #4
Numan Siddique <nusiddiq@redhat.com> writes:

> On Mon, May 27, 2019, 4:01 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
>  On Mon, May 27, 2019 at 11:57 AM 0-day Robot <robot@bytheb.org> wrote:
>  >
>  > Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out your patch.
>  > Thanks for your contribution.
>  >
>  > I encountered some error that I wasn't expecting.  See the details below.
>  >
>  >
>  > git-am:
>  > fatal: sha1 information is lacking or useless (ovn/controller/bfd.c).
>  > Repository lacks necessary blobs to fall back on 3-way merge.
>  > Cannot fall back to three-way merge.
>  > Patch failed at 0001 ovn-controller: Fix parsing of OVN tunnel IDs
>  > The copy of the patch that failed is found in:
>  >    /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch
>  > When you have resolved this problem, run "git am --resolved".
>  > If you prefer to skip this patch, run "git am --skip" instead.
>  > To restore the original branch and stop patching, run "git am --abort".
>  >
>  >
>  > Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>  >
>  > Thanks,
>  > 0-day Robot
>
>  Hi Aaron,
>
>  I'm not sure what the problem is here. Can it be related to what the
>  bot is doing?
>
>  For the v3 version I had updated the master branch and rebased the dev
>  branch; then I generated the patch from my dev branch with:
>  git format-patch -M master --subject-prefix="PATCH v3"  -o _patch/
>
>  If i apply the patch with "git am" everything seems to work fine:
>  $ git checkout -b test origin/master
>  $ git am _patch/0001-ovn-controller-Fix-parsing-of-OVN-tunnel-IDs.patch
>  Applying: ovn-controller: Fix parsing of OVN tunnel IDs
>  $ git log --oneline -1
>  8c559d4 ovn-controller: Fix parsing of OVN tunnel IDs
>  $ git --version
>  git version 1.8.3.1
>
> Looks like the bot is trying to apply the patches to the ovn-org/ovn repo.

I'll double check - I thought I had a regex that was correctly
separating it.

> Aaron is it possible to change the bot to apply the patches to the new repo only if "ovn" is with in the [*ovn*] ?

That's the intention.

> Right now we use  -  "[PATCH ..]ovn: ... " for OVN related patches in the openvswitch repo.

Right.

> Thanks
> Numan
>
>  If I use git-pw it also seems to work fine:
>  $ git checkout -b test2 origin/master
>  Branch test2 set up to track remote branch master from origin.
>  Switched to a new branch 'test2'
>  $ git-pw patch apply 1105697
>  Starting new HTTPS connection (1): patchwork.ozlabs.org
>  Starting new HTTPS connection (1): patchwork.ozlabs.org
>  Applying: ovn-controller: Fix parsing of OVN tunnel IDs
>  $ git log --oneline -1
>  aa83849 ovn-controller: Fix parsing of OVN tunnel IDs
>
>  Thanks,
>  Dumitru
>  _______________________________________________
>  dev mailing list
>  dev@openvswitch.org
>  https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole May 28, 2019, 2:21 p.m. UTC | #5
Aaron Conole <aconole@bytheb.org> writes:

> Numan Siddique <nusiddiq@redhat.com> writes:
>
>> On Mon, May 27, 2019, 4:01 PM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>>  On Mon, May 27, 2019 at 11:57 AM 0-day Robot <robot@bytheb.org> wrote:
>>  >
>>  > Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out your patch.
>>  > Thanks for your contribution.
>>  >
>>  > I encountered some error that I wasn't expecting.  See the details below.
>>  >
>>  >
>>  > git-am:
>>  > fatal: sha1 information is lacking or useless (ovn/controller/bfd.c).
>>  > Repository lacks necessary blobs to fall back on 3-way merge.
>>  > Cannot fall back to three-way merge.
>>  > Patch failed at 0001 ovn-controller: Fix parsing of OVN tunnel IDs
>>  > The copy of the patch that failed is found in:
>>  >    /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch
>>  > When you have resolved this problem, run "git am --resolved".
>>  > If you prefer to skip this patch, run "git am --skip" instead.
>>  > To restore the original branch and stop patching, run "git am --abort".
>>  >
>>  >
>>  > Please check this out.  If you feel there has been an error,
>>  > please email aconole@bytheb.org
>>  >
>>  > Thanks,
>>  > 0-day Robot
>>
>>  Hi Aaron,
>>
>>  I'm not sure what the problem is here. Can it be related to what the
>>  bot is doing?
>>
>>  For the v3 version I had updated the master branch and rebased the dev
>>  branch; then I generated the patch from my dev branch with:
>>  git format-patch -M master --subject-prefix="PATCH v3"  -o _patch/
>>
>>  If i apply the patch with "git am" everything seems to work fine:
>>  $ git checkout -b test origin/master
>>  $ git am _patch/0001-ovn-controller-Fix-parsing-of-OVN-tunnel-IDs.patch
>>  Applying: ovn-controller: Fix parsing of OVN tunnel IDs
>>  $ git log --oneline -1
>>  8c559d4 ovn-controller: Fix parsing of OVN tunnel IDs
>>  $ git --version
>>  git version 1.8.3.1
>>
>> Looks like the bot is trying to apply the patches to the ovn-org/ovn repo.
>
> I'll double check - I thought I had a regex that was correctly
> separating it.

Should be all set.

>> Aaron is it possible to change the bot to apply the patches to the
>> new repo only if "ovn" is with in the [*ovn*] ?
>
> That's the intention.
>
>> Right now we use  -  "[PATCH ..]ovn: ... " for OVN related patches in the openvswitch repo.
>
> Right.
>
>> Thanks
>> Numan
>>
>>  If I use git-pw it also seems to work fine:
>>  $ git checkout -b test2 origin/master
>>  Branch test2 set up to track remote branch master from origin.
>>  Switched to a new branch 'test2'
>>  $ git-pw patch apply 1105697
>>  Starting new HTTPS connection (1): patchwork.ozlabs.org
>>  Starting new HTTPS connection (1): patchwork.ozlabs.org
>>  Applying: ovn-controller: Fix parsing of OVN tunnel IDs
>>  $ git log --oneline -1
>>  aa83849 ovn-controller: Fix parsing of OVN tunnel IDs
>>
>>  Thanks,
>>  Dumitru
>>  _______________________________________________
>>  dev mailing list
>>  dev@openvswitch.org
>>  https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff June 7, 2019, 4:41 p.m. UTC | #6
On Mon, May 27, 2019 at 10:55:52AM +0200, Dumitru Ceara wrote:
> Encap tunnel-ids are of the form:
> <chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
> In physical_run we were checking if a tunnel-id corresponds
> to the local chassis-id by searching if the chassis-id string
> is included in the tunnel-id (strstr). This can break quite
> easily, for example, if the local chassis-id is a substring
> of a remote chassis-id. In that case we were wrongfully
> skipping the tunnel creation.
> 
> To fix that new tunnel-id creation and parsing functions are added in
> encaps.[ch]. These functions are now used everywhere where applicable.
> 
> Acked-by: Venu Iyer <iyervl@ymail.com>
> Reported-at: https://bugzilla.redhat.com/1708131
> Reported-by: Haidong Li <haili@redhat.com>
> Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
> 
> v3: Rebase
> v2: Update commit message

This seems about right.

Here's an incremental with some suggestions for encaps_tunnel_id_parse()
and encaps_tunnel_id_match().  I'm pretty happy with the
encaps_tunnel_id_parse() change; the encaps_tunnel_id_match() one might
be going over the top.

Let me know what you think.

diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index 61b3eab3971f..3b3921a736e2 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -102,64 +102,51 @@ encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
  * If the 'encap_ip' argument is not NULL the function will allocate memory
  * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
  */
-bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
-                            char **encap_ip)
+bool
+encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
+                       char **encap_ip)
 {
-    const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
-
-    if (!match) {
+    /* Find the delimiter.  Fail if there is no delimiter or if <chassis_name>
+     * or <IP> is the empty string.*/
+    const char *d = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
+    if (d == tunnel_id || !d || !d[1]) {
         return false;
     }
 
     if (chassis_id) {
-        size_t chassis_id_len = (match - tunnel_id);
-
-        *chassis_id = xmemdup0(tunnel_id, chassis_id_len);
+        *chassis_id = xmemdup0(tunnel_id, d - tunnel_id);
     }
-
-    /* Consume the tunnel-id delimiter. */
-    match++;
-
     if (encap_ip) {
-        /*
-         * If the value has morphed into something other than
-         * <chassis-id><delim><encap-ip>, fail and free already allocated
-         * memory (i.e., chassis_id).
-         */
-        if (*match == 0) {
-            if (chassis_id) {
-                free(*chassis_id);
-            }
-            return false;
-        }
-        *encap_ip = xstrdup(match);
+        *encap_ip = xstrdup(d + 1);
     }
-
     return true;
 }
 
 /*
- * Returns true if a given tunnel_id contains 'chassis_id' and, if specified,
- * the given 'encap_ip'. Returns false otherwise.
+ * Returns true if 'tunnel_id' contains 'chassis_id' and, if specified, the
+ * given 'encap_ip'. Returns false otherwise.
  */
-bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
-                            const char *encap_ip)
+bool
+encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
+                       const char *encap_ip)
 {
-    if (strstr(tunnel_id, chassis_id) != tunnel_id) {
-        return false;
-    }
-
-    size_t chassis_id_len = strlen(chassis_id);
-
-    if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) {
-        return false;
-    }
-
-    if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) {
-        return false;
+    while (*tunnel_id == *chassis_id) {
+        if (!*tunnel_id) {
+            /* 'tunnel_id' and 'chassis_id' are equal strings.  This is a
+             * mismatch because 'tunnel_id' is missing the delimiter and IP. */
+            return false;
+        }
+        tunnel_id++;
+        chassis_id++;
     }
 
-    return true;
+    /* We found the first byte that disagrees between 'tunnel_id' and
+     * 'chassis_id'.  If we consumed all of 'chassis_id' and arrived at the
+     * delimiter in 'tunnel_id' (and if 'encap_ip' is correct, if it was
+     * supplied), it's a match. */
+    return (*tunnel_id == OVN_MVTEP_CHASSISID_DELIM
+            && *chassis_id == '\0'
+            && (!encap_ip || !strcmp(tunnel_id + 1, encap_ip)));
 }
 
 static void
Dumitru Ceara June 11, 2019, 10:08 a.m. UTC | #7
On Fri, Jun 7, 2019 at 6:42 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Mon, May 27, 2019 at 10:55:52AM +0200, Dumitru Ceara wrote:
> > Encap tunnel-ids are of the form:
> > <chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
> > In physical_run we were checking if a tunnel-id corresponds
> > to the local chassis-id by searching if the chassis-id string
> > is included in the tunnel-id (strstr). This can break quite
> > easily, for example, if the local chassis-id is a substring
> > of a remote chassis-id. In that case we were wrongfully
> > skipping the tunnel creation.
> >
> > To fix that new tunnel-id creation and parsing functions are added in
> > encaps.[ch]. These functions are now used everywhere where applicable.
> >
> > Acked-by: Venu Iyer <iyervl@ymail.com>
> > Reported-at: https://bugzilla.redhat.com/1708131
> > Reported-by: Haidong Li <haili@redhat.com>
> > Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > ---
> >
> > v3: Rebase
> > v2: Update commit message
>
> This seems about right.
>
> Here's an incremental with some suggestions for encaps_tunnel_id_parse()
> and encaps_tunnel_id_match().  I'm pretty happy with the
> encaps_tunnel_id_parse() change; the encaps_tunnel_id_match() one might
> be going over the top.
>
> Let me know what you think.

Thanks for the suggestions.
I like the new encaps_tunnel_id_parse() as it's more succinct and even
though encaps_tunnel_id_match() might be going a bit over the top it's
the most efficient way to do it indeed.

Thanks,
Dumitru

>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index 61b3eab3971f..3b3921a736e2 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -102,64 +102,51 @@ encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
>   * If the 'encap_ip' argument is not NULL the function will allocate memory
>   * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
>   */
> -bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> -                            char **encap_ip)
> +bool
> +encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> +                       char **encap_ip)
>  {
> -    const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> -
> -    if (!match) {
> +    /* Find the delimiter.  Fail if there is no delimiter or if <chassis_name>
> +     * or <IP> is the empty string.*/
> +    const char *d = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> +    if (d == tunnel_id || !d || !d[1]) {
>          return false;
>      }
>
>      if (chassis_id) {
> -        size_t chassis_id_len = (match - tunnel_id);
> -
> -        *chassis_id = xmemdup0(tunnel_id, chassis_id_len);
> +        *chassis_id = xmemdup0(tunnel_id, d - tunnel_id);
>      }
> -
> -    /* Consume the tunnel-id delimiter. */
> -    match++;
> -
>      if (encap_ip) {
> -        /*
> -         * If the value has morphed into something other than
> -         * <chassis-id><delim><encap-ip>, fail and free already allocated
> -         * memory (i.e., chassis_id).
> -         */
> -        if (*match == 0) {
> -            if (chassis_id) {
> -                free(*chassis_id);
> -            }
> -            return false;
> -        }
> -        *encap_ip = xstrdup(match);
> +        *encap_ip = xstrdup(d + 1);
>      }
> -
>      return true;
>  }
>
>  /*
> - * Returns true if a given tunnel_id contains 'chassis_id' and, if specified,
> - * the given 'encap_ip'. Returns false otherwise.
> + * Returns true if 'tunnel_id' contains 'chassis_id' and, if specified, the
> + * given 'encap_ip'. Returns false otherwise.
>   */
> -bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> -                            const char *encap_ip)
> +bool
> +encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> +                       const char *encap_ip)
>  {
> -    if (strstr(tunnel_id, chassis_id) != tunnel_id) {
> -        return false;
> -    }
> -
> -    size_t chassis_id_len = strlen(chassis_id);
> -
> -    if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) {
> -        return false;
> -    }
> -
> -    if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) {
> -        return false;
> +    while (*tunnel_id == *chassis_id) {
> +        if (!*tunnel_id) {
> +            /* 'tunnel_id' and 'chassis_id' are equal strings.  This is a
> +             * mismatch because 'tunnel_id' is missing the delimiter and IP. */
> +            return false;
> +        }
> +        tunnel_id++;
> +        chassis_id++;
>      }
>
> -    return true;
> +    /* We found the first byte that disagrees between 'tunnel_id' and
> +     * 'chassis_id'.  If we consumed all of 'chassis_id' and arrived at the
> +     * delimiter in 'tunnel_id' (and if 'encap_ip' is correct, if it was
> +     * supplied), it's a match. */
> +    return (*tunnel_id == OVN_MVTEP_CHASSISID_DELIM
> +            && *chassis_id == '\0'
> +            && (!encap_ip || !strcmp(tunnel_id + 1, encap_ip)));
>  }
>
>  static void
Dumitru Ceara June 12, 2019, 8:05 a.m. UTC | #8
On Tue, Jun 11, 2019 at 12:08 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On Fri, Jun 7, 2019 at 6:42 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Mon, May 27, 2019 at 10:55:52AM +0200, Dumitru Ceara wrote:
> > > Encap tunnel-ids are of the form:
> > > <chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
> > > In physical_run we were checking if a tunnel-id corresponds
> > > to the local chassis-id by searching if the chassis-id string
> > > is included in the tunnel-id (strstr). This can break quite
> > > easily, for example, if the local chassis-id is a substring
> > > of a remote chassis-id. In that case we were wrongfully
> > > skipping the tunnel creation.
> > >
> > > To fix that new tunnel-id creation and parsing functions are added in
> > > encaps.[ch]. These functions are now used everywhere where applicable.
> > >
> > > Acked-by: Venu Iyer <iyervl@ymail.com>
> > > Reported-at: https://bugzilla.redhat.com/1708131
> > > Reported-by: Haidong Li <haili@redhat.com>
> > > Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> > > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > > ---
> > >
> > > v3: Rebase
> > > v2: Update commit message
> >
> > This seems about right.
> >
> > Here's an incremental with some suggestions for encaps_tunnel_id_parse()
> > and encaps_tunnel_id_match().  I'm pretty happy with the
> > encaps_tunnel_id_parse() change; the encaps_tunnel_id_match() one might
> > be going over the top.
> >
> > Let me know what you think.
>
> Thanks for the suggestions.
> I like the new encaps_tunnel_id_parse() as it's more succinct and even
> though encaps_tunnel_id_match() might be going a bit over the top it's
> the most efficient way to do it indeed.
>
> Thanks,
> Dumitru

Hi Ben,

Shall I send a v4 incorporating your suggestions or will you directly
apply your incremental patch with the changes on top of mine?

Thanks,
Dumitru

>
> >
> > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> > index 61b3eab3971f..3b3921a736e2 100644
> > --- a/ovn/controller/encaps.c
> > +++ b/ovn/controller/encaps.c
> > @@ -102,64 +102,51 @@ encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
> >   * If the 'encap_ip' argument is not NULL the function will allocate memory
> >   * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
> >   */
> > -bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> > -                            char **encap_ip)
> > +bool
> > +encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> > +                       char **encap_ip)
> >  {
> > -    const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> > -
> > -    if (!match) {
> > +    /* Find the delimiter.  Fail if there is no delimiter or if <chassis_name>
> > +     * or <IP> is the empty string.*/
> > +    const char *d = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> > +    if (d == tunnel_id || !d || !d[1]) {
> >          return false;
> >      }
> >
> >      if (chassis_id) {
> > -        size_t chassis_id_len = (match - tunnel_id);
> > -
> > -        *chassis_id = xmemdup0(tunnel_id, chassis_id_len);
> > +        *chassis_id = xmemdup0(tunnel_id, d - tunnel_id);
> >      }
> > -
> > -    /* Consume the tunnel-id delimiter. */
> > -    match++;
> > -
> >      if (encap_ip) {
> > -        /*
> > -         * If the value has morphed into something other than
> > -         * <chassis-id><delim><encap-ip>, fail and free already allocated
> > -         * memory (i.e., chassis_id).
> > -         */
> > -        if (*match == 0) {
> > -            if (chassis_id) {
> > -                free(*chassis_id);
> > -            }
> > -            return false;
> > -        }
> > -        *encap_ip = xstrdup(match);
> > +        *encap_ip = xstrdup(d + 1);
> >      }
> > -
> >      return true;
> >  }
> >
> >  /*
> > - * Returns true if a given tunnel_id contains 'chassis_id' and, if specified,
> > - * the given 'encap_ip'. Returns false otherwise.
> > + * Returns true if 'tunnel_id' contains 'chassis_id' and, if specified, the
> > + * given 'encap_ip'. Returns false otherwise.
> >   */
> > -bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> > -                            const char *encap_ip)
> > +bool
> > +encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> > +                       const char *encap_ip)
> >  {
> > -    if (strstr(tunnel_id, chassis_id) != tunnel_id) {
> > -        return false;
> > -    }
> > -
> > -    size_t chassis_id_len = strlen(chassis_id);
> > -
> > -    if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) {
> > -        return false;
> > -    }
> > -
> > -    if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) {
> > -        return false;
> > +    while (*tunnel_id == *chassis_id) {
> > +        if (!*tunnel_id) {
> > +            /* 'tunnel_id' and 'chassis_id' are equal strings.  This is a
> > +             * mismatch because 'tunnel_id' is missing the delimiter and IP. */
> > +            return false;
> > +        }
> > +        tunnel_id++;
> > +        chassis_id++;
> >      }
> >
> > -    return true;
> > +    /* We found the first byte that disagrees between 'tunnel_id' and
> > +     * 'chassis_id'.  If we consumed all of 'chassis_id' and arrived at the
> > +     * delimiter in 'tunnel_id' (and if 'encap_ip' is correct, if it was
> > +     * supplied), it's a match. */
> > +    return (*tunnel_id == OVN_MVTEP_CHASSISID_DELIM
> > +            && *chassis_id == '\0'
> > +            && (!encap_ip || !strcmp(tunnel_id + 1, encap_ip)));
> >  }
> >
> >  static void
Ben Pfaff June 12, 2019, 3:46 p.m. UTC | #9
On Wed, Jun 12, 2019 at 10:05:36AM +0200, Dumitru Ceara wrote:
> On Tue, Jun 11, 2019 at 12:08 PM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > On Fri, Jun 7, 2019 at 6:42 PM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > On Mon, May 27, 2019 at 10:55:52AM +0200, Dumitru Ceara wrote:
> > > > Encap tunnel-ids are of the form:
> > > > <chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
> > > > In physical_run we were checking if a tunnel-id corresponds
> > > > to the local chassis-id by searching if the chassis-id string
> > > > is included in the tunnel-id (strstr). This can break quite
> > > > easily, for example, if the local chassis-id is a substring
> > > > of a remote chassis-id. In that case we were wrongfully
> > > > skipping the tunnel creation.
> > > >
> > > > To fix that new tunnel-id creation and parsing functions are added in
> > > > encaps.[ch]. These functions are now used everywhere where applicable.
> > > >
> > > > Acked-by: Venu Iyer <iyervl@ymail.com>
> > > > Reported-at: https://bugzilla.redhat.com/1708131
> > > > Reported-by: Haidong Li <haili@redhat.com>
> > > > Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > > > ---
> > > >
> > > > v3: Rebase
> > > > v2: Update commit message
> > >
> > > This seems about right.
> > >
> > > Here's an incremental with some suggestions for encaps_tunnel_id_parse()
> > > and encaps_tunnel_id_match().  I'm pretty happy with the
> > > encaps_tunnel_id_parse() change; the encaps_tunnel_id_match() one might
> > > be going over the top.
> > >
> > > Let me know what you think.
> >
> > Thanks for the suggestions.
> > I like the new encaps_tunnel_id_parse() as it's more succinct and even
> > though encaps_tunnel_id_match() might be going a bit over the top it's
> > the most efficient way to do it indeed.
> >
> > Thanks,
> > Dumitru
> 
> Hi Ben,
> 
> Shall I send a v4 incorporating your suggestions or will you directly
> apply your incremental patch with the changes on top of mine?

If you like the changes, or some of them, you can send a v4
incorporating them.  If you don't like them, let me know and I'll look
at v3 again.
Dumitru Ceara June 12, 2019, 4:01 p.m. UTC | #10
On Wed, Jun 12, 2019 at 5:47 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Wed, Jun 12, 2019 at 10:05:36AM +0200, Dumitru Ceara wrote:
> > On Tue, Jun 11, 2019 at 12:08 PM Dumitru Ceara <dceara@redhat.com> wrote:
> > >
> > > On Fri, Jun 7, 2019 at 6:42 PM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > On Mon, May 27, 2019 at 10:55:52AM +0200, Dumitru Ceara wrote:
> > > > > Encap tunnel-ids are of the form:
> > > > > <chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
> > > > > In physical_run we were checking if a tunnel-id corresponds
> > > > > to the local chassis-id by searching if the chassis-id string
> > > > > is included in the tunnel-id (strstr). This can break quite
> > > > > easily, for example, if the local chassis-id is a substring
> > > > > of a remote chassis-id. In that case we were wrongfully
> > > > > skipping the tunnel creation.
> > > > >
> > > > > To fix that new tunnel-id creation and parsing functions are added in
> > > > > encaps.[ch]. These functions are now used everywhere where applicable.
> > > > >
> > > > > Acked-by: Venu Iyer <iyervl@ymail.com>
> > > > > Reported-at: https://bugzilla.redhat.com/1708131
> > > > > Reported-by: Haidong Li <haili@redhat.com>
> > > > > Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > > > > ---
> > > > >
> > > > > v3: Rebase
> > > > > v2: Update commit message
> > > >
> > > > This seems about right.
> > > >
> > > > Here's an incremental with some suggestions for encaps_tunnel_id_parse()
> > > > and encaps_tunnel_id_match().  I'm pretty happy with the
> > > > encaps_tunnel_id_parse() change; the encaps_tunnel_id_match() one might
> > > > be going over the top.
> > > >
> > > > Let me know what you think.
> > >
> > > Thanks for the suggestions.
> > > I like the new encaps_tunnel_id_parse() as it's more succinct and even
> > > though encaps_tunnel_id_match() might be going a bit over the top it's
> > > the most efficient way to do it indeed.
> > >
> > > Thanks,
> > > Dumitru
> >
> > Hi Ben,
> >
> > Shall I send a v4 incorporating your suggestions or will you directly
> > apply your incremental patch with the changes on top of mine?
>
> If you like the changes, or some of them, you can send a v4
> incorporating them.  If you don't like them, let me know and I'll look
> at v3 again.

Done, just sent a v4:
https://patchwork.ozlabs.org/patch/1114661/

Thanks,
Dumitru

Patch
diff mbox series

diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c
index a2f590d..22db00a 100644
--- a/ovn/controller/bfd.c
+++ b/ovn/controller/bfd.c
@@ -15,6 +15,7 @@ 
 
 #include <config.h>
 #include "bfd.h"
+#include "encaps.h"
 #include "lport.h"
 #include "ovn-controller.h"
 
@@ -72,14 +73,16 @@  bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int,
                         const char *id = smap_get(&port_rec->external_ids,
                                                   "ovn-chassis-id");
                         if (id) {
-                            char *chassis_name;
-                            char *save_ptr = NULL;
-                            char *tokstr = xstrdup(id);
-                            chassis_name = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, &save_ptr);
-                            if (chassis_name && !sset_contains(active_tunnels, chassis_name)) {
-                                sset_add(active_tunnels, chassis_name);
+                            char *chassis_name = NULL;
+
+                            if (encaps_tunnel_id_parse(id, &chassis_name,
+                                                       NULL)) {
+                                if (!sset_contains(active_tunnels,
+                                                   chassis_name)) {
+                                    sset_add(active_tunnels, chassis_name);
+                                }
+                                free(chassis_name);
                             }
-                            free(tokstr);
                         }
                     }
                 }
@@ -193,17 +196,17 @@  bfd_run(const struct ovsrec_interface_table *interface_table,
         const char *tunnel_id = smap_get(&br_int->ports[k]->external_ids,
                                           "ovn-chassis-id");
         if (tunnel_id) {
-            char *chassis_name;
-            char *save_ptr = NULL;
-            char *tokstr = xstrdup(tunnel_id);
+            char *chassis_name = NULL;
             char *port_name = br_int->ports[k]->name;
 
             sset_add(&tunnels, port_name);
-            chassis_name = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, &save_ptr);
-            if (chassis_name && sset_contains(&bfd_chassis, chassis_name)) {
-                sset_add(&bfd_ifaces, port_name);
+
+            if (encaps_tunnel_id_parse(tunnel_id, &chassis_name, NULL)) {
+                if (sset_contains(&bfd_chassis, chassis_name)) {
+                    sset_add(&bfd_ifaces, port_name);
+                }
+                free(chassis_name);
             }
-            free(tokstr);
         }
     }
 
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index 7bd52d1..61b3eab 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -26,6 +26,13 @@ 
 
 VLOG_DEFINE_THIS_MODULE(encaps);
 
+/*
+ * Given there could be multiple tunnels with different IPs to the same
+ * chassis we annotate the ovn-chassis-id with
+ * <chassis_name>OVN_MVTEP_CHASSISID_DELIM<IP>.
+ */
+#define	OVN_MVTEP_CHASSISID_DELIM '@'
+
 void
 encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 {
@@ -78,6 +85,83 @@  tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id)
     return NULL;
 }
 
+/*
+ * Returns a tunnel-id of the form 'chassis_id'-delimiter-'encap_ip'.
+ */
+char *
+encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
+{
+    return xasprintf("%s%c%s", chassis_id, OVN_MVTEP_CHASSISID_DELIM,
+                     encap_ip);
+}
+
+/*
+ * Parses a 'tunnel_id' of the form <chassis_name><delimiter><IP>.
+ * If the 'chassis_id' argument is not NULL the function will allocate memory
+ * and store the chassis-id part of the tunnel-id at '*chassis_id'.
+ * If the 'encap_ip' argument is not NULL the function will allocate memory
+ * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
+ */
+bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
+                            char **encap_ip)
+{
+    const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
+
+    if (!match) {
+        return false;
+    }
+
+    if (chassis_id) {
+        size_t chassis_id_len = (match - tunnel_id);
+
+        *chassis_id = xmemdup0(tunnel_id, chassis_id_len);
+    }
+
+    /* Consume the tunnel-id delimiter. */
+    match++;
+
+    if (encap_ip) {
+        /*
+         * If the value has morphed into something other than
+         * <chassis-id><delim><encap-ip>, fail and free already allocated
+         * memory (i.e., chassis_id).
+         */
+        if (*match == 0) {
+            if (chassis_id) {
+                free(*chassis_id);
+            }
+            return false;
+        }
+        *encap_ip = xstrdup(match);
+    }
+
+    return true;
+}
+
+/*
+ * Returns true if a given tunnel_id contains 'chassis_id' and, if specified,
+ * the given 'encap_ip'. Returns false otherwise.
+ */
+bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
+                            const char *encap_ip)
+{
+    if (strstr(tunnel_id, chassis_id) != tunnel_id) {
+        return false;
+    }
+
+    size_t chassis_id_len = strlen(chassis_id);
+
+    if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) {
+        return false;
+    }
+
+    if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) {
+        return false;
+    }
+
+    return true;
+}
+
 static void
 tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
            const char *new_chassis_id, const struct sbrec_encap *encap)
@@ -94,8 +178,7 @@  tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
      * combination of the chassis_name and the encap-ip to identify
      * a specific tunnel to the chassis.
      */
-    tunnel_entry_id = xasprintf("%s%s%s", new_chassis_id,
-                                OVN_MVTEP_CHASSISID_DELIM, encap->ip);
+    tunnel_entry_id = encaps_tunnel_id_create(new_chassis_id, encap->ip);
     if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) {
         smap_add(&options, "csum", csum);
     }
diff --git a/ovn/controller/encaps.h b/ovn/controller/encaps.h
index 7ed3e09..afa4183 100644
--- a/ovn/controller/encaps.h
+++ b/ovn/controller/encaps.h
@@ -39,4 +39,10 @@  void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
                     const struct ovsrec_bridge *br_int);
 
+char *encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip);
+bool  encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
+                             char **encap_ip);
+bool  encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
+                             const char *encap_ip);
+
 #endif /* ovn/encaps.h */
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index 6afd727..2c224c8 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -81,11 +81,4 @@  enum chassis_tunnel_type {
 
 uint32_t get_tunnel_type(const char *name);
 
-/*
- * Given there could be multiple tunnels with different IPs to the same
- * chassis we annotate the ovn-chassis-id with
- * <chassis_name>OVN_MVTEP_CHASSISID_DELIM<IP>.
- */
-#define	OVN_MVTEP_CHASSISID_DELIM	"@"
-
 #endif /* ovn/ovn-controller.h */
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index c8dc282..dd455dd 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -16,6 +16,7 @@ 
 #include <config.h>
 #include "binding.h"
 #include "byte-order.h"
+#include "encaps.h"
 #include "flow.h"
 #include "ha-chassis.h"
 #include "lflow.h"
@@ -80,38 +81,27 @@  struct chassis_tunnel {
 /*
  * This function looks up the list of tunnel ports (provided by
  * ovn-chassis-id ports) and returns the tunnel for the given chassid-id and
- * encap-ip. The ovn-chassis-id is formed using the chassis-id and encap-ip as
- * <chassis-id>OVN_MVTEP_CHASSISID_DELIM<encap-ip>. The list is hashed using
- * the chassis-id. If the encap-ip is not specified, it means we'll just
- * return a tunnel for that chassis-id, i.e. we just check for chassis-id and
- * if there is a match, we'll return the tunnel. If encap-ip is also provided we
- * use <chassis-id>OVN_MVTEP_CHASSISID_DELIM<encap-ip> to do a more specific
- * lookup.
+ * encap-ip. The ovn-chassis-id is formed using the chassis-id and encap-ip.
+ * The list is hashed using the chassis-id. If the encap-ip is not specified,
+ * it means we'll just return a tunnel for that chassis-id, i.e. we just check
+ * for chassis-id and if there is a match, we'll return the tunnel.
+ * If encap-ip is also provided we use both chassis-id and encap-ip to do
+ * a more specific lookup.
  */
 static struct chassis_tunnel *
 chassis_tunnel_find(const char *chassis_id, char *encap_ip)
 {
-    char *chassis_tunnel_entry;
-
     /*
      * If the specific encap_ip is given, look for the chassisid_ip entry,
      * else return the 1st found entry for the chassis.
      */
-    if (encap_ip != NULL) {
-        chassis_tunnel_entry = xasprintf("%s%s%s", chassis_id,
-            OVN_MVTEP_CHASSISID_DELIM, encap_ip);
-    } else {
-        chassis_tunnel_entry = xasprintf("%s", chassis_id);
-    }
     struct chassis_tunnel *tun = NULL;
     HMAP_FOR_EACH_WITH_HASH (tun, hmap_node, hash_string(chassis_id, 0),
                              &tunnels) {
-        if (strstr(tun->chassis_id, chassis_tunnel_entry) != NULL) {
-            free (chassis_tunnel_entry);
+        if (encaps_tunnel_id_match(tun->chassis_id, chassis_id, encap_ip)) {
             return tun;
         }
     }
-    free (chassis_tunnel_entry);
     return NULL;
 }
 
@@ -1064,8 +1054,9 @@  physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
         }
 
         const char *tunnel_id = smap_get(&port_rec->external_ids,
-                                          "ovn-chassis-id");
-        if (tunnel_id && strstr(tunnel_id, chassis->name)) {
+                                         "ovn-chassis-id");
+        if (tunnel_id &&
+                encaps_tunnel_id_match(tunnel_id, chassis->name, NULL)) {
             continue;
         }
 
@@ -1121,16 +1112,10 @@  physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                  * where we need to tunnel to the chassis, but won't
                  * have the encap-ip specifically.
                  */
-                char *tokstr = xstrdup(tunnel_id);
-                char *save_ptr = NULL;
-                char *hash_id = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM,
-                                &save_ptr);
-                char *ip = strtok_r(NULL, "", &save_ptr);
-                /*
-                 * If the value has morphed into something other than
-                 * chassis-id>delim>encap-ip, ignore.
-                 */
-                if (!hash_id || !ip) {
+                char *hash_id = NULL;
+                char *ip = NULL;
+
+                if (!encaps_tunnel_id_parse(tunnel_id, &hash_id, &ip)) {
                     continue;
                 }
                 struct chassis_tunnel *tun = chassis_tunnel_find(hash_id, ip);
@@ -1151,7 +1136,8 @@  physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                     tun->type = tunnel_type;
                     physical_map_changed = true;
                 }
-                free(tokstr);
+                free(hash_id);
+                free(ip);
                 break;
             } else {
                 const char *iface_id = smap_get(&iface_rec->external_ids,
@@ -1256,7 +1242,8 @@  physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name,
             struct match match = MATCH_CATCHALL_INITIALIZER;
 
             if (!binding->chassis ||
-                strstr(tun->chassis_id, binding->chassis->name) == NULL) {
+                !encaps_tunnel_id_match(tun->chassis_id,
+                                        binding->chassis->name, NULL)) {
                 continue;
             }
 
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index b7bb4c9..798bd34 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -22,6 +22,7 @@ 
 #include "csum.h"
 #include "dirs.h"
 #include "dp-packet.h"
+#include "encaps.h"
 #include "flow.h"
 #include "ha-chassis.h"
 #include "lport.h"
@@ -2725,7 +2726,8 @@  get_localnet_vifs_l3gwports(
         }
         const char *tunnel_id = smap_get(&port_rec->external_ids,
                                           "ovn-chassis-id");
-        if (tunnel_id && strstr(tunnel_id, chassis->name)) {
+        if (tunnel_id &&
+                encaps_tunnel_id_match(tunnel_id, chassis->name, NULL)) {
             continue;
         }
         const char *localnet = smap_get(&port_rec->external_ids,