On Tue, Jun 11, 2019 at 12:08 PM Dumitru Ceara <[email protected]> wrote:
>
> On Fri, Jun 7, 2019 at 6:42 PM Ben Pfaff <[email protected]> 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 <[email protected]>
> > > Reported-at: https://bugzilla.redhat.com/1708131
> > > Reported-by: Haidong Li <[email protected]>
> > > Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> > > Signed-off-by: Dumitru Ceara <[email protected]>
> > > ---
> > >
> > > 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
