Hi Ben, Sorry again! Attached the patch to email initially.
Submitted a pull request for the same issue. https://github.com/openvswitch/ovs/pull/269 Thanks, Prashanth -----Original Message----- From: Ben Pfaff <b...@ovn.org> Sent: Wednesday, January 16, 2019 15:24 To: Iyengar, Prashanth <prashanth_iyen...@alliedtelesis.com> Cc: d...@openvswitch.org; Aaron Conole <acon...@redhat.com>; Gupta, Rahul <rahul_gu...@alliedtelesis.com>; Katagiri, Junichi <junichi_katag...@alliedtelesis.com>; van der Peet, Tony <tony.vanderp...@alliedtelesis.co.nz> Subject: Re: [ovs-dev] Fix OpenFlow v1.3.4 Conformance test failures: 430.500, 430.510 The attachment looks missing to me. On Wed, Jan 16, 2019 at 10:29:39PM +0000, Iyengar, Prashanth wrote: > Hi Aaron, > > Apologies for the formatting issues earlier. > Attached the patch to the message here as suggested. > Let me know if this works. Otherwise, will look to submit a pull request. > > Sorry for the inconvenience. > > Thanks, > Prashanth > > -----Original Message----- > From: Aaron Conole <acon...@redhat.com> > Sent: Wednesday, January 16, 2019 06:05 > To: Iyengar, Prashanth <prashanth_iyen...@alliedtelesis.com> > Cc: d...@openvswitch.org; Gupta, Rahul <rahul_gu...@alliedtelesis.com>; > Katagiri, Junichi <junichi_katag...@alliedtelesis.com>; van der Peet, > Tony <tony.vanderp...@alliedtelesis.co.nz> > Subject: Re: [ovs-dev] Fix OpenFlow v1.3.4 Conformance test failures: > 430.500, 430.510 > > "Iyengar, Prashanth" <prashanth_iyen...@alliedtelesis.com> writes: > > > From fc0a9e1329573692c04438370f85e87125a268b0 Mon Sep 17 00:00:00 > > 2001 > > From: Prashanth Iyengar <prashanth_iyen...@alliedtelesis.com> > > Date: Mon, 14 Jan 2019 12:53:11 -0800 > > Subject: [PATCH] Fix OpenFlow v1.3.4 Conf test failures: 430.500, > > 430.510 > > > > This commit adds additional verification to nx_pull_header__() in > > lib/nx-match.c to distinguish between bad match and bad action header > > conditions and return the appropriate error type/code. > > > > Signed-off-by: Prashanth Iyengar > > <prashanth_iyen...@alliedtelesis.com> > > Reviewed-by: Tony van der Peet <tony.vanderp...@alliedtelesis.co.nz> > > Reviewed-by: Rahul Gupta <rahul_gu...@alliedtelesis.com> > > --- > > There seems to be some formatting issues not just with the code, but also > with this particular patch. If it continues to be an issue, consider either > attaching the patch to the message, or submitting a pull request on github > and notifying the list. > > > lib/meta-flow.c | 2 +- > > lib/nx-match.c | 62 +++++++++++++++++++++++++++++++++++++------- > > lib/nx-match.h | 2 +- > > tests/ofp-actions.at | 14 ++++++++++ > > 4 files changed, 68 insertions(+), 12 deletions(-) > > > > diff --git a/lib/meta-flow.c b/lib/meta-flow.c index > > b6d9e92b6..8f9662943 100644 > > --- a/lib/meta-flow.c > > +++ b/lib/meta-flow.c > > @@ -3503,7 +3503,7 @@ mf_vl_mff_nx_pull_entry(struct ofpbuf *b, const > > struct vl_mff_map *vl_mff_map, > > const struct mf_field **field, union mf_value > > *value, > > union mf_value *mask, uint64_t *tlv_bitmap) { > > - enum ofperr error = nx_pull_entry(b, vl_mff_map, field, value, mask); > > + enum ofperr error = nx_pull_entry(b, vl_mff_map, field, value, > > + mask, true); > > The spacing here isn't correct. > > > if (error) { > > return error; > > } > > diff --git a/lib/nx-match.c b/lib/nx-match.c index > > fd3eac0c0..33b95a2d4 100644 > > --- a/lib/nx-match.c > > +++ b/lib/nx-match.c > > @@ -97,6 +97,7 @@ enum ofp12_oxm_class { > > > > /* Functions for extracting raw field values from OXM/NXM headers. > > */ static uint32_t nxm_vendor(uint64_t header) { return header; } > > +enum ofperr nxm_validate_action_field_header(uint64_t header); > > static int nxm_class(uint64_t header) { return header >> 48; } > > static int nxm_field(uint64_t header) { return (header >> 41) & > > 0x7f; } static bool nxm_hasmask(uint64_t header) { return (header > > >> 40) & 1; } @@ -313,7 +314,7 @@ is_cookie_pseudoheader(uint64_t > > header) static enum ofperr nx_pull_header__(struct ofpbuf *b, bool > > allow_cookie, > > Looks like a number of lines got collapsed? > > > const struct vl_mff_map *vl_mff_map, uint64_t *header, > > - const struct mf_field **field) > > + const struct mf_field **field, bool is_action) > > { > > if (b->size < 4) { > > goto bad_len; > > @@ -340,7 +341,16 @@ nx_pull_header__(struct ofpbuf *b, bool allow_cookie, > > if (!*field && !(allow_cookie && is_cookie_pseudoheader(*header))) > > { > > VLOG_DBG_RL(&rl, "OXM header "NXM_HEADER_FMT" is unknown", > > NXM_HEADER_ARGS(*header)); > > - return OFPERR_OFPBMC_BAD_FIELD; > > + if (is_action) { > > + enum ofperr h_error = > > nxm_validate_action_field_header(*header); > > + if (h_error) { > > + *field = NULL; > > + return h_error; > > + } > > + return OFPERR_OFPBAC_BAD_SET_TYPE; > > + } else { > > + return OFPERR_OFPBMC_BAD_FIELD; > > + } > > } else if (mf_vl_mff_invalid(*field, vl_mff_map)) { > > return OFPERR_NXFMFC_INVALID_TLV_FIELD; > > } > > @@ -381,7 +391,7 @@ static enum ofperr nx_pull_entry__(struct > > ofpbuf *b, bool allow_cookie, > > const struct vl_mff_map *vl_mff_map, uint64_t *header, > > const struct mf_field **field_, > > - union mf_value *value, union mf_value *mask) > > + union mf_value *value, union mf_value *mask, bool > > + is_action) > > Likewise, spacing > > > { > > const struct mf_field *field; > > enum ofperr header_error; > > @@ -390,7 +400,7 @@ nx_pull_entry__(struct ofpbuf *b, bool allow_cookie, > > int width; > > > > header_error = nx_pull_header__(b, allow_cookie, vl_mff_map, header, > > - &field); > > + &field, is_action); > > if (header_error && header_error != OFPERR_OFPBMC_BAD_FIELD) { > > return header_error; > > } > > @@ -442,15 +452,20 @@ nx_pull_entry__(struct ofpbuf *b, bool allow_cookie, > > * > > * If a NULL 'mask' is supplied, masked OXM or NXM entries are treated as > > * errors (with OFPERR_OFPBMC_BAD_MASK). > > + * > > + * The "bool is_action" is supplied to differentiate between match > > + and action > > + * headers. This is done in order to return appropriate error type > > + and code for > > + * bad match or bad action conditions. If set to True, indicates > > + that the > > + * OXM or NXM entries belong to an action header. > > */ > > enum ofperr > > nx_pull_entry(struct ofpbuf *b, const struct vl_mff_map *vl_mff_map, > > const struct mf_field **field, union mf_value *value, > > - union mf_value *mask) > > + union mf_value *mask, bool is_action) > > { > > uint64_t header; > > > > - return nx_pull_entry__(b, false, vl_mff_map, &header, field, value, > > mask); > > + return nx_pull_entry__(b, false, vl_mff_map, &header, field, > > + value, mask, is_action); > > } > > > > /* Attempts to pull an NXM or OXM header from the beginning of 'b'. If @@ > > -470,7 +485,7 @@ nx_pull_header(struct ofpbuf *b, const struct vl_mff_map > > *vl_mff_map, > > enum ofperr error; > > uint64_t header; > > > > - error = nx_pull_header__(b, false, vl_mff_map, &header, field); > > + error = nx_pull_header__(b, false, vl_mff_map, &header, field, > > + false); > > if (masked) { > > *masked = !error && nxm_hasmask(header); > > } else if (!error && nxm_hasmask(header)) { @@ -489,7 +504,7 @@ > > nx_pull_match_entry(struct ofpbuf *b, bool allow_cookie, > > Something weird here. > > > uint64_t header; > > > > error = nx_pull_entry__(b, allow_cookie, vl_mff_map, &header, field, > > value, > > - mask); > > + mask, false); > > if (error) { > > return error; > > } > > @@ -739,7 +754,7 @@ oxm_pull_field_array(const void *fields_data, size_t > > fields_len, > > uint64_t header; > > > > error = nx_pull_entry__(&b, false, NULL, &header, &field, &value, > > - NULL); > > + NULL, false); > > if (error) { > > VLOG_DBG_RL(&rl, "error pulling field array field"); > > } else if (!field) { > > @@ -1488,7 +1503,7 @@ nx_match_to_string(const uint8_t *p, unsigned int > > match_len) > > uint64_t header; > > int value_len; > > > > - error = nx_pull_entry__(&b, true, NULL, &header, NULL, &value, > > &mask); > > + error = nx_pull_entry__(&b, true, NULL, &header, NULL, > > + &value, &mask, false); > > if (error) { > > break; > > } > > @@ -2244,6 +2259,33 @@ nxm_field_by_header(uint64_t header) > > return NULL; > > } > > > > +enum ofperr > > +nxm_validate_action_field_header(uint64_t header) { > > > Please reformat as: > > +enum ofperr nxm_validate_action_field_header(uint64_t header) { > > > + const struct nxm_field_index *nfi; > > + uint64_t header_no_len; > > + > > + nxm_init(); > > + if (nxm_hasmask(header)) { > > + header = nxm_make_exact_header(header); > > + } > > + > > + header_no_len = nxm_no_len(header); > > + > > + HMAP_FOR_EACH_IN_BUCKET (nfi, header_node, hash_uint64(header_no_len), > > + &nxm_header_map) { > > + if (header_no_len == nxm_no_len(nfi->nf.header)) { > > + if (nxm_length(header) > 0) { > > + if (nxm_length(header) != nxm_length(nfi->nf.header)) { > > + return OFPERR_OFPBAC_BAD_SET_LEN; > > + } > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > static const struct nxm_field * > > nxm_field_by_name(const char *name, size_t len) { diff --git > > a/lib/nx-match.h b/lib/nx-match.h index e304072d3..9be40a981 100644 > > Something weird here with the patch... > > > --- a/lib/nx-match.h > > +++ b/lib/nx-match.h > > @@ -82,7 +82,7 @@ int oxm_put_field_array(struct ofpbuf *, const struct > > field_array *, > > * ID followed by a value and possibly a mask). */ enum ofperr > > nx_pull_entry(struct ofpbuf *, const struct vl_mff_map *, > > const struct mf_field **, union mf_value *value, > > - union mf_value *mask); > > + union mf_value *mask, bool is_action); > > enum ofperr nx_pull_header(struct ofpbuf *, const struct vl_mff_map *, > > const struct mf_field **, bool *masked); > > void nxm_put_entry_raw(struct ofpbuf *, enum mf_field_id field, diff > > --git a/tests/ofp-actions.at b/tests/ofp-actions.at index > > e320a92a8..38c2b5cbd 100644 > > --- a/tests/ofp-actions.at > > +++ b/tests/ofp-actions.at > > @@ -746,6 +746,20 @@ dnl Check the ONF extension form of "copy_field". > > # actions=move:NXM_OF_IN_PORT[]->NXM_OF_VLAN_TCI[] > > ffff 0020 4f4e4600 0c80 0000 0010 0000 0000 0000 00000002 00000802 > > 00000000 > > > > +dnl Check OpenFlow v1.3.4 Conformance Test: 430.500. > > +# bad OpenFlow13 actions: OFPBAC_BAD_SET_TYPE & > > +ofp_actions|WARN|bad action at offset 0 (OFPBAC_BAD_SET_TYPE): > > +& 00000000 00 19 00 08 80 00 fe 00-00 00 00 10 00 00 00 01 & > > +00000010 > > +00 00 00 00 00 00 00 00- > > +0019 0008 8000fe00 000000100000 000100000000 00000000 > > + > > +dnl Check OpenFlow v1.3.4 Conformance Test: 430.510. > > +# bad OpenFlow13 actions: OFPBAC_BAD_SET_LEN & ofp_actions|WARN|bad > > +action at offset 0 (OFPBAC_BAD_SET_LEN): > > +& 00000000 00 19 00 10 80 00 08 07-00 01 02 03 04 05 00 00 & > > +00000010 > > +00 00 00 10 00 00 00 01- > > +0019 0010 80000807 000102030405 000000000010 00000001 > > + > > ]) > > sed '/^[[#&]]/d' < test-data > input.txt sed -n 's/^# //p; /^$/p' > > < test-data > expout > > -- > > 2.17.1 > > > > This e-mail message is for the sole use of the intended recipient(s) and > > may contain confidential and privileged information. Any unauthorized > > review, use, disclosure or distribution is prohibited. If you are not the > > intended recipient, please contact the sender by reply E-mail and destroy > > all copies of the original message. If you are the intended recipient, > > please be advised that the content of this message is subject to access, > > review and disclosure by the sender's E-mail System Administrator. > > > > Privacy Policy > > These trailers aren't appropriate on a public mailing list. > > > We are committed to respecting your privacy and keeping you informed > > about our practices. Read our Privacy Policy: Allied Telesis Privacy > > Policy<http://www.alliedtelesis.com/legal/privacy> > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > This e-mail message is for the sole use of the intended recipient(s) and may > contain confidential and privileged information. Any unauthorized review, > use, disclosure or distribution is prohibited. If you are not the intended > recipient, please contact the sender by reply E-mail and destroy all copies > of the original message. If you are the intended recipient, please be advised > that the content of this message is subject to access, review and disclosure > by the sender's E-mail System Administrator. > > Privacy Policy > > We are committed to respecting your privacy and keeping you informed > about our practices. Read our Privacy Policy: Allied Telesis Privacy > Policy<http://www.alliedtelesis.com/legal/privacy> > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev This e-mail message is for the sole use of the intended recipient(s) and may contain confidential and privileged information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply E-mail and destroy all copies of the original message. If you are the intended recipient, please be advised that the content of this message is subject to access, review and disclosure by the sender's E-mail System Administrator. Privacy Policy We are committed to respecting your privacy and keeping you informed about our practices. Read our Privacy Policy: Allied Telesis Privacy Policy<http://www.alliedtelesis.com/legal/privacy> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
