Hi, I think that removing the padding should be the duty of the packet parsing library. Previously, my code was receiving packets with trailing 0 bytes at the end as the payload of the IP packet. That was incorrect and it'd be easy to forget to check the total_length every time I receive a packet. I think it'd be much better to put that logic in the packet parsing library where it can't be forgotten on any code path.
The patch works for any protocol that defines the total_length field, for other packets, the old behavior is used. I think the only implemented packet type that currently requires this special candling is IPV4, though: * arp doesn't carry a payload so the 0-bytes are already ignored; arguably, you could define total_length = MIN_LENGTH to make sure that the same packet is always generated * vlan/mpls doesn't carry a length so the algorithm will examine the encapsulated packet; if the encapsulated packet is ipv4 then it should get truncated properly as the algorithm works down the chain * tcp/udp/icmp is carried inside ipv4 so any truncation should happen outside the tcp packet * when ipv6 is added, we should add the total_length field to ensure it is handled appropriately * when gre is added, the encapsulated gre packet is wrapped in a valid ipv4 packet, the outer ipv4 packet will trigger truncation, which should leave nothing to be done when the inner gre/ipv4 packet is reached. I'm less sure about adding the padding to the packet, you're right that the underlying network driver should add that padding. It just seemed like the library should be symmetrical in adding/removing padding. -Shaun On 24/05/2013 20:23, "Isaku Yamahata" <[email protected]> wrote: >Hi. Thank you for splitting the patch and quick respin. > >Can you please elaborate your motivation and concrete issue >you're addressing on? >Although it's obvious for you, it's not clear for me. (or >useless/harmful.) >The commit message describes only what, not why. >Why do you need to pad etherframe to minimul size by Ryu packet library? >Why do you need to truncate padding by Ryu packet library? > >Regarding to adding padding: >Open vSwitch pads etherframe (more exactly Linux NIC driver does), so it >is unnecessary to pad etherframe by OF controller. So it is OpenFlow >switch >that is responsible to pad etherframe. Not controller. > >In case of tunneling like transparent ethernet GRE tunneling, >(ryu packet library doesn't support it yet, though) >the sequence of protocols is '[ether ip gre ether]'. So automatic blind >padding can harm. > >Regarding to truncating padding: >Your patch is specific to ipv4. It is easy to determine payload size >based on total_length. So I think keeping padding doesn't matter. >How does it matter to you? > >thanks, > >On Fri, May 24, 2013 at 05:07:12PM +0000, Shaun Crampton wrote: >> From: Shaun Crampton <[email protected]> >> >> Add support for ethernet padding. >> >> * Add padding when serializing small packets. >> * Modify packet.py to support adding a footer as well as header >> in back-compatible way. If only header is returned, packet >> behaves as before. >> * When packets supply a total_length field, truncate the packet >> to that length in order to remove any additional padding. >> When no total_length is specified, act as before. >> >> Signed-off-by: Shaun Crampton <[email protected]> >> >> --- >> Our use case was to interoperate with a switch that included the >>ethernet >> padding when forwarding a packet to the controller. We needed to remove >> the padding before forwarding to our control plane software. >> >> ryu/lib/packet/ethernet.py | 9 +++++++-- >> ryu/lib/packet/icmp.py | 1 - >> ryu/lib/packet/packet.py | 20 +++++++++++++++++++- >> 3 files changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/ryu/lib/packet/ethernet.py b/ryu/lib/packet/ethernet.py >> index 7bc18ef..0d9c2a9 100644 >> --- a/ryu/lib/packet/ethernet.py >> +++ b/ryu/lib/packet/ethernet.py >> @@ -51,8 +51,13 @@ class ethernet(packet_base.PacketBase): >> return cls(dst, src, ethertype), >> ethernet.get_packet_type(ethertype) >> >> def serialize(self, payload, prev): >> - return struct.pack(ethernet._PACK_STR, self.dst, self.src, >> - self.ethertype) >> + header = struct.pack(ethernet._PACK_STR, self.dst, self.src, >> + self.ethertype) >> + total_len = len(header) + len(payload) >> + # The minimum size of an ethernet frame is 64 bytes, including >>the >> + # 4-byte CRC, which is added by the hardware. Pad the packet. >> + pad_len = max(60 - total_len, 0) >> + return header, "\0" * pad_len >> >> >> # copy vlan _TYPES >> diff --git a/ryu/lib/packet/icmp.py b/ryu/lib/packet/icmp.py >> index e42fa40..819e9ab 100644 >> --- a/ryu/lib/packet/icmp.py >> +++ b/ryu/lib/packet/icmp.py >> @@ -79,7 +79,6 @@ class icmp(packet_base.PacketBase): >> msg.data = cls_.parser(buf, offset) >> else: >> msg.data = buf[offset:] >> - >> return msg, None >> >> def serialize(self, payload, prev): >> diff --git a/ryu/lib/packet/packet.py b/ryu/lib/packet/packet.py >> index f5fd82d..5602be2 100644 >> --- a/ryu/lib/packet/packet.py >> +++ b/ryu/lib/packet/packet.py >> @@ -44,6 +44,17 @@ class Packet(object): >> while cls: >> proto, cls = cls.parser(self.data[self.parsed_bytes:]) >> if proto: >> + try: >> + total_length = proto.total_length >> + except Exception: >> + pass >> + else: >> + if total_length + self.parsed_bytes < >>len(self.data): >> + # The data remaining is longer that the header >>of >> the >> + # packet says it should be. We likely have >>some >> + # ethernet padding added. Truncate it. >> + truncation_length = total_length + >> self.parsed_bytes >> + self.data = self.data[:truncation_length] >> self.parsed_bytes += proto.length >> self.protocols.append(proto) >> >> @@ -67,7 +78,14 @@ class Packet(object): >> data = p.serialize(self.data, prev) >> else: >> data = str(p) >> - self.data = data + self.data >> + try: >> + # Special-case: some protocols add a footer as well as >>a >> + # header. >> + prefix, suffix = data >> + except Exception: >> + prefix = data >> + suffix = "" >> + self.data = prefix + self.data + suffix >> >> def add_protocol(self, proto): >> """Register a protocol *proto* for this packet. >> -- >> 1.7.9.5 >> >> >> >>------------------------------------------------------------------------- >>----- >> Try New Relic Now & We'll Send You this Cool Shirt >> New Relic is the only SaaS-based application performance monitoring >>service >> that delivers powerful full stack analytics. Optimize and monitor your >> browser, app, & servers with just a few lines of code. Try New Relic >> and get this awesome Nerd Life shirt! >>http://p.sf.net/sfu/newrelic_d2d_may >> _______________________________________________ >> Ryu-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/ryu-devel >> > >-- >yamahata ------------------------------------------------------------------------------ Try New Relic Now & We'll Send You this Cool Shirt New Relic is the only SaaS-based application performance monitoring service that delivers powerful full stack analytics. Optimize and monitor your browser, app, & servers with just a few lines of code. Try New Relic and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may _______________________________________________ Ryu-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ryu-devel
