| Message ID | 1488846170-23012-16-git-send-email-jarno@ovn.org |
|---|---|
| State | Accepted |
| Delegated to: | Joe Stringer |
| Headers | show |
On 6 March 2017 at 16:22, Jarno Rajahalme <jarno@ovn.org> wrote: > The decoder of packet_in messages should not fail on encountering > unknown metadata fields. This allows the switch to add new features > without breaking controllers. The controllers should, however, copy > the metadata fields from the packet_int to packet_out so that the > switch gets back the full metadata. OVN is already doing this. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > --- > lib/nx-match.c | 25 ++++++++++++++++--------- > lib/nx-match.h | 4 ++-- > lib/ofp-util.c | 5 +++-- > 3 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/lib/nx-match.c b/lib/nx-match.c > index 95516a1..bcc1347 100644 > --- a/lib/nx-match.c > +++ b/lib/nx-match.c > @@ -504,6 +504,9 @@ nx_pull_match_entry(struct ofpbuf *b, bool allow_cookie, > return 0; > } > > +/* Prerequisites will only be checked when 'strict' is 'true'. This allows > + * decoding conntrack original direction 5-tuple IP addresses without the > + * ethertype being present, when decoding metadata only. */ > static enum ofperr > nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict, > struct match *match, ovs_be64 *cookie, ovs_be64 *cookie_mask, > @@ -539,7 +542,7 @@ nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict, > *cookie = value.be64; > *cookie_mask = mask.be64; > } > - } else if (!mf_are_match_prereqs_ok(field, match)) { > + } else if (strict && !mf_are_match_prereqs_ok(field, match)) { > error = OFPERR_OFPBMC_BAD_PREREQ; > } else if (!mf_is_all_wild(field, &match->wc)) { > error = OFPERR_OFPBMC_DUP_FIELD; > @@ -607,7 +610,8 @@ nx_pull_match(struct ofpbuf *b, unsigned int match_len, struct match *match, > } > > /* Behaves the same as nx_pull_match(), but skips over unknown NXM headers, > - * instead of failing with an error. */ > + * instead of failing with an error, and does not check for field > + * prerequisities. */ > enum ofperr > nx_pull_match_loose(struct ofpbuf *b, unsigned int match_len, > struct match *match, > @@ -664,8 +668,9 @@ oxm_pull_match(struct ofpbuf *b, const struct tun_table *tun_table, > return oxm_pull_match__(b, true, tun_table, match); > } > > -/* Behaves the same as oxm_pull_match() with one exception. Skips over unknown > - * OXM headers instead of failing with an error when they are encountered. */ > +/* Behaves the same as oxm_pull_match() with two exceptions. Skips over > + * unknown OXM headers instead of failing with an error when they are > + * encountered, and does not check for field prerequisities. */ > enum ofperr > oxm_pull_match_loose(struct ofpbuf *b, const struct tun_table *tun_table, > struct match *match) > @@ -676,14 +681,16 @@ oxm_pull_match_loose(struct ofpbuf *b, const struct tun_table *tun_table, > /* Parses the OXM match description in the 'oxm_len' bytes in 'oxm'. Stores > * the result in 'match'. > * > - * Fails with an error when encountering unknown OXM headers. > + * Returns 0 if successful, otherwise an OpenFlow error code. > * > - * Returns 0 if successful, otherwise an OpenFlow error code. */ > + * Encountering unknown OXM headers or missing field prerequisites are not > + * considered as error conditions. > + */ > enum ofperr > -oxm_decode_match(const void *oxm, size_t oxm_len, > - const struct tun_table *tun_table, struct match *match) > +oxm_decode_match_loose(const void *oxm, size_t oxm_len, > + const struct tun_table *tun_table, struct match *match) > { > - return nx_pull_raw(oxm, oxm_len, true, match, NULL, NULL, tun_table); > + return nx_pull_raw(oxm, oxm_len, false, match, NULL, NULL, tun_table); > } > > /* Verify an array of OXM TLVs treating value of each TLV as a mask, > diff --git a/lib/nx-match.h b/lib/nx-match.h > index 631ab48..b599731 100644 > --- a/lib/nx-match.h > +++ b/lib/nx-match.h > @@ -61,8 +61,8 @@ enum ofperr oxm_pull_match(struct ofpbuf *, const struct tun_table *, > struct match *); > enum ofperr oxm_pull_match_loose(struct ofpbuf *, const struct tun_table *, > struct match *); > -enum ofperr oxm_decode_match(const void *, size_t, const struct tun_table *, > - struct match *); > +enum ofperr oxm_decode_match_loose(const void *, size_t, > + const struct tun_table *, struct match *); > enum ofperr oxm_pull_field_array(const void *, size_t fields_len, > struct field_array *); > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index 7d40cbb..ed66dd1 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -3397,8 +3397,9 @@ decode_nx_packet_in2(const struct ofp_header *oh, bool loose, > } > > case NXPINT_METADATA: > - error = oxm_decode_match(payload.msg, ofpbuf_msgsize(&payload), > - tun_table, &pin->flow_metadata); > + error = oxm_decode_match_loose(payload.msg, > + ofpbuf_msgsize(&payload), > + tun_table, &pin->flow_metadata); Should we use 'loose' to determine whether to strictly decode or ignore unknown OXMs?
diff --git a/lib/nx-match.c b/lib/nx-match.c index 95516a1..bcc1347 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -504,6 +504,9 @@ nx_pull_match_entry(struct ofpbuf *b, bool allow_cookie, return 0; } +/* Prerequisites will only be checked when 'strict' is 'true'. This allows + * decoding conntrack original direction 5-tuple IP addresses without the + * ethertype being present, when decoding metadata only. */ static enum ofperr nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict, struct match *match, ovs_be64 *cookie, ovs_be64 *cookie_mask, @@ -539,7 +542,7 @@ nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict, *cookie = value.be64; *cookie_mask = mask.be64; } - } else if (!mf_are_match_prereqs_ok(field, match)) { + } else if (strict && !mf_are_match_prereqs_ok(field, match)) { error = OFPERR_OFPBMC_BAD_PREREQ; } else if (!mf_is_all_wild(field, &match->wc)) { error = OFPERR_OFPBMC_DUP_FIELD; @@ -607,7 +610,8 @@ nx_pull_match(struct ofpbuf *b, unsigned int match_len, struct match *match, } /* Behaves the same as nx_pull_match(), but skips over unknown NXM headers, - * instead of failing with an error. */ + * instead of failing with an error, and does not check for field + * prerequisities. */ enum ofperr nx_pull_match_loose(struct ofpbuf *b, unsigned int match_len, struct match *match, @@ -664,8 +668,9 @@ oxm_pull_match(struct ofpbuf *b, const struct tun_table *tun_table, return oxm_pull_match__(b, true, tun_table, match); } -/* Behaves the same as oxm_pull_match() with one exception. Skips over unknown - * OXM headers instead of failing with an error when they are encountered. */ +/* Behaves the same as oxm_pull_match() with two exceptions. Skips over + * unknown OXM headers instead of failing with an error when they are + * encountered, and does not check for field prerequisities. */ enum ofperr oxm_pull_match_loose(struct ofpbuf *b, const struct tun_table *tun_table, struct match *match) @@ -676,14 +681,16 @@ oxm_pull_match_loose(struct ofpbuf *b, const struct tun_table *tun_table, /* Parses the OXM match description in the 'oxm_len' bytes in 'oxm'. Stores * the result in 'match'. * - * Fails with an error when encountering unknown OXM headers. + * Returns 0 if successful, otherwise an OpenFlow error code. * - * Returns 0 if successful, otherwise an OpenFlow error code. */ + * Encountering unknown OXM headers or missing field prerequisites are not + * considered as error conditions. + */ enum ofperr -oxm_decode_match(const void *oxm, size_t oxm_len, - const struct tun_table *tun_table, struct match *match) +oxm_decode_match_loose(const void *oxm, size_t oxm_len, + const struct tun_table *tun_table, struct match *match) { - return nx_pull_raw(oxm, oxm_len, true, match, NULL, NULL, tun_table); + return nx_pull_raw(oxm, oxm_len, false, match, NULL, NULL, tun_table); } /* Verify an array of OXM TLVs treating value of each TLV as a mask, diff --git a/lib/nx-match.h b/lib/nx-match.h index 631ab48..b599731 100644 --- a/lib/nx-match.h +++ b/lib/nx-match.h @@ -61,8 +61,8 @@ enum ofperr oxm_pull_match(struct ofpbuf *, const struct tun_table *, struct match *); enum ofperr oxm_pull_match_loose(struct ofpbuf *, const struct tun_table *, struct match *); -enum ofperr oxm_decode_match(const void *, size_t, const struct tun_table *, - struct match *); +enum ofperr oxm_decode_match_loose(const void *, size_t, + const struct tun_table *, struct match *); enum ofperr oxm_pull_field_array(const void *, size_t fields_len, struct field_array *); diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 7d40cbb..ed66dd1 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -3397,8 +3397,9 @@ decode_nx_packet_in2(const struct ofp_header *oh, bool loose, } case NXPINT_METADATA: - error = oxm_decode_match(payload.msg, ofpbuf_msgsize(&payload), - tun_table, &pin->flow_metadata); + error = oxm_decode_match_loose(payload.msg, + ofpbuf_msgsize(&payload), + tun_table, &pin->flow_metadata); break; case NXPINT_USERDATA:
The decoder of packet_in messages should not fail on encountering unknown metadata fields. This allows the switch to add new features without breaking controllers. The controllers should, however, copy the metadata fields from the packet_int to packet_out so that the switch gets back the full metadata. OVN is already doing this. Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- lib/nx-match.c | 25 ++++++++++++++++--------- lib/nx-match.h | 4 ++-- lib/ofp-util.c | 5 +++-- 3 files changed, 21 insertions(+), 13 deletions(-)