On Wed, Sep 18, 2013 at 01:42:41PM -0700, Jarno Rajahalme wrote:
> From ovs-ofctl man page:
>
> tcp_flags=flags/mask
> Bitwise match on TCP flags. The flags and mask are 16-bit
> num???
> bers written in decimal or in hexadecimal prefixed by 0x. Each
> 1-bit in mask requires that the corresponding bit in port must
> match. Each 0-bit in mask causes the corresponding bit to be
> ignored.
>
> TCP protocol currently defines 9 flag bits, and additional 3
> bits are reserved (must be transmitted as zero), see RFCs 793,
> 3168, and 3540. The flags are, numbering from the least
> signif???
> icant bit:
>
> 1: FIN No more data from sender.
>
> 2: SYN Synchronize sequence numbers.
>
> 3: RST Reset the connection.
>
> 4: PSH Push function.
>
> 5: ACK Acknowledgement field significant.
>
> 6: URG Urgent pointer field significant.
>
> 7: ECE ECN Echo.
>
> 8: CWR Congestion Windows Reduced.
>
> 9: NS Nonce Sum.
>
> 10-12: Reserved.
>
> 13-16: Not matchable, must be zero in both flags and mask.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
I'm used to seeing bits numbered starting from 0, not 1. Starting
from zero there's the natural correspondence that bit n has value 2**n
or 1<<n. The relationship is less natural starting from 1.
It's unusual to require the mask bits to be 0. This precludes
exact-matching. I don't think we do that for any other fields.
This patch needs a review from the kernel-side maintainers. I only
skimmed the kernel code and didn't have any comments.
I think you can use is_ip_any() here:
> + if ((f->dl_type == htons(ETH_TYPE_IP)
> + || f->dl_type == htons(ETH_TYPE_IPV6)) &&
> + f->nw_proto == IPPROTO_TCP) {
> + if (wc->masks.tcp_flags) {
> + if (wc->masks.tcp_flags == htons(UINT16_MAX)) {
I slightly prefer %# over 0x% in the following cases, because 0 looks
a little less silly than 0x0:
> + ds_put_format(s, "tcp_flags=0x%03"PRIx16",",
> + ntohs(f->tcp_flags));
> + } else {
> + ds_put_format(s, "tcp_flags=0x%03"PRIx16"/0x%03"PRIx16",",
> + ntohs(f->tcp_flags),
> + ntohs(wc->masks.tcp_flags));
> + }
> + }
> + }
>
> if (s->length > start_len && ds_last(s) == ',') {
> s->length--;
Here, isn't OFPUTIL_P_ANY too loose? OF1.0 and OF1.1 don't support
TCP flags matching:
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -559,6 +559,17 @@ static const struct mf_field mf_fields[MFF_N_IDS] = {
> OXM_OF_TCP_DST, "OXM_OF_TCP_DST",
> OFPUTIL_P_ANY,
> OFPUTIL_P_NXM_OXM_ANY,
> + }, {
> + MFF_TCP_FLAGS, "tcp_flags", NULL,
> + 2, 12,
> + MFM_FULLY,
> + MFS_HEXADECIMAL,
> + MFP_TCP,
> + false,
> + NXM_NX_TCP_FLAGS, "NXM_NX_TCP_FLAGS",
> + NXM_NX_TCP_FLAGS, "NXM_NX_TCP_FLAGS",
> + OFPUTIL_P_ANY,
> + OFPUTIL_P_NXM_OXM_ANY,
> },
0x%# looks wrong, you'll get output like 0x0x123:
> + ds_put_format(ds, "0x%#"PRIx16, ntohs(nl_attr_get_be16(a)));
> + if (!is_exact) {
> + ds_put_format(ds, "/0x%#"PRIx16, ntohs(nl_attr_get_be16(ma)));
> + }
> + break;
> +
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
