This looks good to me, though I wonder if you'd be willing to do a little more
while you're at it. Good POX code should always use a literal tuple when using
the string formatting operator. This may not actually be on the wiki, but it
should be. The packet library predates POX and its conventions, though, so
there's plenty of code in there which breaks the rule.
So basically, things like...
self.msg('(ip parse) warning: IP version %u not IPv4' % self.v)
should be...
self.msg('(ip parse) warning: IP version %u not IPv4' % (self.v,))
Or, better yet, I think the following would work:
self.msg('(ip parse) warning: IP version %u not IPv4', self.v)
And for that matter, I don't think there's any reason not to do:
self.msg('(ip parse) warning: IP version %s not IPv4', self.v)
.. which is less likely to have an attempt at a helpful log message result in
an exception instead.
If you don't have the time and inclination, let me know. Just seemed to me
like as long as you're fixing things... might as well fix them the rest of the
way.
(And I'll respond on the subject of addresses.py eventually; just haven't quite
had the time to yet.)
-- Murphy
On Jul 22, 2014, at 1:05 AM, Peter Peresini <peter.peres...@epfl.ch> wrote:
> Hi Murphy,
> here is a small patch that improves warning messages generated during IPv4
> parsing.
>
> Peter
>
>
> From 7b5f395bd49696290d647b08db271a4ca0e06e43 Mon Sep 17 00:00:00 2001
> From: Peter Peresini <peter.peres...@gmail.com>
> Date: Tue, 22 Jul 2014 10:02:18 +0200
> Subject: [PATCH] Improved warning messages during IPv4 parsing
>
> ---
> pox/lib/packet/ipv4.py | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/pox/lib/packet/ipv4.py b/pox/lib/packet/ipv4.py
> index 3eb4ae6..6c47a2e 100644
> --- a/pox/lib/packet/ipv4.py
> +++ b/pox/lib/packet/ipv4.py
> @@ -123,19 +123,22 @@ class ipv4(packet_base):
> self.srcip = IPAddr(self.srcip)
>
> if self.v != ipv4.IPv4:
> - self.msg('(ip parse) warning IP version %u not IPv4' % self.v)
> + self.msg('(ip parse) warning: IP version %u not IPv4' % self.v)
> return
> - elif self.hl < 5:
> - self.msg('(ip parse) warning IP header %u longer than len %u' \
> - % (self.hl, self.iplen))
> + if self.hl < 5:
> + self.msg('(ip parse) warning: IP header length shorter than
> MIN_LEN (IHL=%u => header len=%u)' \
> + % (self.hl, 4 * self.hl))
> return
> - elif self.iplen < ipv4.MIN_LEN:
> - self.msg('(ip parse) warning invalid IP len %u' % self.iplen)
> + if self.iplen < ipv4.MIN_LEN:
> + self.msg('(ip parse) warning: Invalid IP len %u' % self.iplen)
> return
> - elif (self.hl * 4) >= self.iplen or (self.hl * 4) > dlen:
> - self.msg('(ip parse) warning IP header %u longer than len %u' \
> + if (self.hl * 4) > self.iplen:
> + self.msg('(ip parse) warning: IP header longer than IP length
> including payload (%u vs %u)' \
> % (self.hl, self.iplen))
> return
> + if (self.hl * 4) > dlen:
> + self.msg('(ip parse) warning: IP header is truncated')
> + return
>
> # At this point, we are reasonably certain that we have an IP
> # packet
> --
> 1.8.3.2
>
