On Sep 24, 2013, at 5:10 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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 seemed a bit unclear to me at first, but reading the OpenFlow spec
again, I see numerous phrases mentioning mask as all ones, so that
must apply even with fields that do not have that many bits.
>
> 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)) {
>
Yes.
> I slightly prefer %# over 0x% in the following cases, because 0 looks
> a little less silly than 0x0:
"%#03" would print zero as "000". My intent was to output the flags as "0xXXX",
where "XXX" are hex digits corresponding to the 12 flags bits, so at to make
the width of the flags field explicit.
However, if you think the flags should be printed without zero padding in the
front, I'm more than happy to change this as you suggested.
>> + 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:
Oops… too much cut&paste :-)
>> --- 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:
Right, wrong. And I have not been consistent here regarding prepending with 0
as above. Let me know what you think about the zero-padding...
>> + 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
