On Tue, May 28, 2013 at 04:54:17PM +0000, Shaun Crampton wrote:
> Hi,
Hi. Now I understand the point.
Let's drop adding padding part and implement truncate padding in clean
way instead of ad-hoc enhance like checking total_length.
How about this
PacketBase
def __init__
...
self.payload_size = 0
parser of subclass of PacketBase sets self.payload_size > 0 if necessary.
Packet._parser truncates padding if proto.payload_size > 0.
And modify related subclasses. ipv4, ipv6, udp...
thanks,
>
> 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
>
--
yamahata
------------------------------------------------------------------------------
Introducing AppDynamics Lite, a free troubleshooting tool for Java/.NET
Get 100% visibility into your production application - at no cost.
Code-level diagnostics for performance bottlenecks with <2% overhead
Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap1
_______________________________________________
Ryu-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ryu-devel
