[Ryu-devel] OFPOxmId in Table Features Request

2017-04-04 Thread William Fisher 
When an OXM id is serialized in a table features request, the OXM
header that is serialized sets the oxm_length to 0.  That is, the OXM
id for "eth_dst" is sent as

8600 instead of 8606 (the last byte is the length of the value).

There's a comment in the code that explains the issue was unclear in
2014. (ofproto_v1_3_parser, ofproto_v1_4_parser, ofproto_v1_5_parser)

The 1.3.5 OF spec states, "The oxm_length field in OXM headers must be
the length value defined for the OXM field, i.e. the payload length if
the OXM field had a payload." (7.3.5.5.2 Table Features properties)

OVS appears to set the oxm_length to the payload length. If the mask
bit is set, the payload length is double the usual size.

I am trying to change the OFPOxmId serialization code so I can submit
a patch. Is there an API like:

ofproto.oxm_from_user('eth_dst', None)

that will return the length information for a field?

The ryu parsing code for an OXM ID ignores the oxm_length. This patch
would only break things if the receiving OpenFlow implementation
expects the oxm_length's to be zero in the TableFeatures message
OXMID's. Such an implementation would be broken IMO.

-Bill

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Ryu-devel mailing list
Ryu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ryu-devel

Re: [Ryu-devel] OFPOxmId in Table Features Request

2017-04-04 Thread Iwase Yusuke 
Hi Bill

Thank you for your report!


On 2017年04月05日 12:20, William Fisher wrote:
> When an OXM id is serialized in a table features request, the OXM
> header that is serialized sets the oxm_length to 0.  That is, the OXM
> id for "eth_dst" is sent as
> 
> 8600 instead of 8606 (the last byte is the length of the value).
> 
> There's a comment in the code that explains the issue was unclear in
> 2014. (ofproto_v1_3_parser, ofproto_v1_4_parser, ofproto_v1_5_parser)
> 
> The 1.3.5 OF spec states, "The oxm_length field in OXM headers must be
> the length value defined for the OXM field, i.e. the payload length if
> the OXM field had a payload." (7.3.5.5.2 Table Features properties)
> 
> OVS appears to set the oxm_length to the payload length. If the mask
> bit is set, the payload length is double the usual size.
> 
> I am trying to change the OFPOxmId serialization code so I can submit
> a patch. Is there an API like:
> 
> ofproto.oxm_from_user('eth_dst', None)
> 
> that will return the length information for a field?

Great!
It looks good to me.

ofproto.oxm_from_user() returns a tuple of ("oxm_type" value, serialized
value, serialized mask).
>>> ofproto.oxm_from_user('ipv4_src', ('192.168.0.0', '255.255.255.0'))
(4194315, b'\xc0\xa8\x00\x00', b'\xff\xff\xff\x00')

Instead, I guess we need to extend ofproto.oxm_serialize_header() to
include the "hasmask" field.
Just an idea, how about the following?

$ git diff
diff --git a/ryu/ofproto/oxx_fields.py b/ryu/ofproto/oxx_fields.py
index e9c1fb9..a3cc1d3 100644
--- a/ryu/ofproto/oxx_fields.py
+++ b/ryu/ofproto/oxx_fields.py
@@ -228,7 +228,7 @@ def _make_exp_hdr(oxx, mod, n):
 return n, exp_hdr
 
 
-def _serialize_header(oxx, mod, n, buf, offset):
+def _serialize_header(oxx, mod, n, buf, offset, hasmask=False):
 try:
 get_desc = getattr(mod, '_' + oxx + '_field_desc')
 desc = get_desc(n)
@@ -238,8 +238,10 @@ def _serialize_header(oxx, mod, n, buf, offset):
 n, exp_hdr = _make_exp_hdr(oxx, mod, n)
 exp_hdr_len = len(exp_hdr)
 pack_str = "!I%ds" % (exp_hdr_len,)
+if hasmask:
+value_len *= 2
 msg_pack_into(pack_str, buf, offset,
-  (n << 9) | (0 << 8) | (exp_hdr_len + value_len),
+  (n << 9) | (hasmask << 8) | (exp_hdr_len + value_len),
   bytes(exp_hdr))
 return struct.calcsize(pack_str)
 

With this change, we can serialize oxm header field as following.

>>> from ryu.ofproto import ofproto_v1_3 as ofproto
>>> n = ofproto.oxm_from_user_header('ipv4_src')
>>> buf = bytearray()
>>> ofproto.oxm_serialize_header(n, buf, offset=0, hasmask=True)
4
>>> buf
bytearray(b'\x80\x00\x17\x08')
>>> ofproto.oxm_serialize_header(n, buf, offset=0, hasmask=False)
4
>>> buf
bytearray(b'\x80\x00\x16\x04')


Thanks,
Iwase

> 
> The ryu parsing code for an OXM ID ignores the oxm_length. This patch
> would only break things if the receiving OpenFlow implementation
> expects the oxm_length's to be zero in the TableFeatures message
> OXMID's. Such an implementation would be broken IMO.
> 
> -Bill
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Ryu-devel mailing list
> Ryu-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ryu-devel
> 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Ryu-devel mailing list
Ryu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ryu-devel

Re: [Ryu-devel] OFPOxmId in Table Features Request

2017-04-05 Thread William Fisher 
Thanks! The change to oxm_serialize_header helped immensely.

I'm working on adjusting the tablefeatures test packet_data.  I'll
also test experimenter oxm id's.

-Bill



On Tue, Apr 4, 2017 at 9:59 PM, Iwase Yusuke  wrote:
> Instead, I guess we need to extend ofproto.oxm_serialize_header() to
> include the "hasmask" field.
> Just an idea, how about the following?
>
> $ git diff
> diff --git a/ryu/ofproto/oxx_fields.py b/ryu/ofproto/oxx_fields.py
> index e9c1fb9..a3cc1d3 100644
> --- a/ryu/ofproto/oxx_fields.py
> +++ b/ryu/ofproto/oxx_fields.py
> @@ -228,7 +228,7 @@ def _make_exp_hdr(oxx, mod, n):
>  return n, exp_hdr
>
>
> -def _serialize_header(oxx, mod, n, buf, offset):
> +def _serialize_header(oxx, mod, n, buf, offset, hasmask=False):
>  try:
>  get_desc = getattr(mod, '_' + oxx + '_field_desc')
>  desc = get_desc(n)
> @@ -238,8 +238,10 @@ def _serialize_header(oxx, mod, n, buf, offset):
>  n, exp_hdr = _make_exp_hdr(oxx, mod, n)
>  exp_hdr_len = len(exp_hdr)
>  pack_str = "!I%ds" % (exp_hdr_len,)
> +if hasmask:
> +value_len *= 2
>  msg_pack_into(pack_str, buf, offset,
> -  (n << 9) | (0 << 8) | (exp_hdr_len + value_len),
> +  (n << 9) | (hasmask << 8) | (exp_hdr_len + value_len),
>bytes(exp_hdr))
>  return struct.calcsize(pack_str)
>
>
> With this change, we can serialize oxm header field as following.
>
 from ryu.ofproto import ofproto_v1_3 as ofproto
 n = ofproto.oxm_from_user_header('ipv4_src')
 buf = bytearray()
 ofproto.oxm_serialize_header(n, buf, offset=0, hasmask=True)
> 4
 buf
> bytearray(b'\x80\x00\x17\x08')
 ofproto.oxm_serialize_header(n, buf, offset=0, hasmask=False)
> 4
 buf
> bytearray(b'\x80\x00\x16\x04')

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Ryu-devel mailing list
Ryu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ryu-devel