Code review - Issue 4440051: Fragmentation support for IPv4https://codereview.appspot.com/2011-07-09T01:25:49+00:00rietveld
Message from tommypec@gmail.com
2011-04-21T00:57:21+00:00Tommaso Pecorellaurn:md5:48ce20ead48ef61be2b48d0c38e2dbcd
Message from tommypec@gmail.com
2011-04-21T11:24:02+00:00Tommaso Pecorellaurn:md5:7627718203e9ea56503325bff6dfae1c
The tentative is not poor anymore.
I completed the fragmentation support (assuming I haven't forgot anything) and it should be ready for evaluation.
Feel free to check it and comment at will.
Cheers,
Tommaso
On 21/apr/2011, at 02.57, tommypec@gmail.com wrote:
> Reviewers: ,
>
> Description:
> The bug here seems related to this:
>
> http://www.nsnam.org/bugzilla/show_bug.cgi?id=1072
>
>
>
> Please review this at http://codereview.appspot.com/4440051/
>
> Affected files:
> A scratch/udp-echo-fragm.cc
> M src/internet/model/ipv4-l3-protocol.cc
> M src/internet/model/ipv4-l3-protocol.h
> M src/network/model/packet-metadata.cc
>
>
Message from tommypec@gmail.com
2011-04-22T17:48:30+00:00Tommaso Pecorellaurn:md5:4bfea45f5faafc20d3f93208027271cc
Message from tomh.org@gmail.com
2011-04-22T21:29:49+00:00Tom Hendersonurn:md5:23a12ae8e46dedac24e65e8eab15b21d
My main comment is that it would be nice to have a few unit/regression tests, and to reconcile with Vedran's implementation. I noticed that Vedran has some tests already written.
it would be nice if you and Vedran could review each other's approach and decide on the best way to handle it, or whether they can be merged somehow. One small benefit I see on this approach vs. Vedran's is that Vedran's changed the API slightly (SendOut vs. SendRealOut)
http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc
File scratch/udp-echo-fragm.cc (right):
http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#newcode26
scratch/udp-echo-fragm.cc:26: // - Tracing of queues and packet receptions to file "udp-echo.tr"
It would be nice to turn this into a simple unit test instead of an example. e.g. nodes n0 <-> n1 interconnected by SimpleNetDevice with MTU of 1500, and send IPv4 packet sizes of e.g. 0, 1000, 1500, 1501, 3000, 3001, 65535.
Then, test fragmentation reordering and loss of certain fragments, and timeout behavior.
http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#newcode117
scratch/udp-echo-fragm.cc:117: // workaround: make sure the ARP cache is valid by sending first a small packet.
Or, bump up the PendingQueueSize in the ArpCache from its default of 3.
http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#newcode120
scratch/udp-echo-fragm.cc:120: // Honestly, don't try.
That is only for IPv6 when Jumbo Payload is implemented. Plus, UDP and TCP must be changed to support it. However, I would expect that we ought to support (test) a 65535-byte IPv4 and IPv6 datagram.
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc
File src/internet/model/ipv4-l3-protocol.cc (right):
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode738
src/internet/model/ipv4-l3-protocol.cc:738: uint32_t fragmentSize = outInterface->GetDevice ()->GetMtu () & ~uint32_t (0x7);
suggest a comment on the above cryptic line
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode1153
src/internet/model/ipv4-l3-protocol.cc:1153: // a much more complex handling is necessary in case there are options.
can you check for options here, and assert if present? (and request in the comment for someone to provide a patch to support it)
playing with IPv4 options is not outside the realm of possibility for a research simulator, so we shouldn't lull the user into believing that it is supported.
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode1320
src/internet/model/ipv4-l3-protocol.cc:1320: // XXX
is this an XXX, or is it just an observation that this implementation does not overwrite or otherwise look for fragmentation attacks?
Message from tommypec@gmail.com
2011-04-22T22:31:57+00:00Tommaso Pecorellaurn:md5:a96a16a91f676f20a7a31fcf427b8c01
On 2011/04/22 21:29:49, Tom Henderson wrote:
> My main comment is that it would be nice to have a few unit/regression tests,
> and to reconcile with Vedran's implementation. I noticed that Vedran has some
> tests already written.
You're right about unit testing. About Vedran's implementation I'm in contact with him and he checked my implementation. We developed separately (my fault, I noticed Vedran's one when I had mine almost complete) but I'll take a deeper look at hi to check if there is some stuff to reconcile.
> it would be nice if you and Vedran could review each other's approach and decide
> on the best way to handle it, or whether they can be merged somehow. One small
> benefit I see on this approach vs. Vedran's is that Vedran's changed the API
> slightly (SendOut vs. SendRealOut)
Vedran already checked this, I asked him to become a reviewer for this. I wouldn't mind either if we adopt Vedran's one, the main goal is to have fragmentation support, not who's the author.
> http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc
> File scratch/udp-echo-fragm.cc (right):
>
> http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#newcode26
> scratch/udp-echo-fragm.cc:26: // - Tracing of queues and packet receptions to
> file "udp-echo.tr"
> It would be nice to turn this into a simple unit test instead of an example.
> [...]
Will do.
> http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#newcode117
> scratch/udp-echo-fragm.cc:117: // workaround: make sure the ARP cache is valid
> by sending first a small packet.
> Or, bump up the PendingQueueSize in the ArpCache from its default of 3.
Nice tip for the unit test.
> http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc
> File src/internet/model/ipv4-l3-protocol.cc (right):
>
> http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode738
> src/internet/model/ipv4-l3-protocol.cc:738: uint32_t fragmentSize =
> outInterface->GetDevice ()->GetMtu () & ~uint32_t (0x7);
> suggest a comment on the above cryptic line
Yep. Fragments must be 8-bytes aligned except for the last one.
> http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode1153
> src/internet/model/ipv4-l3-protocol.cc:1153: // a much more complex handling is
> necessary in case there are options.
> can you check for options here, and assert if present? (and request in the
> comment for someone to provide a patch to support it)
>
> playing with IPv4 options is not outside the realm of possibility for a research
> simulator, so we shouldn't lull the user into believing that it is supported.
Mmm... right. Ns-3 actually don't have IPv4 option headers (so I can't test any support either), but it MUST be clear that if anything will be implemented, this code have to be changed as well.
Maybe I could add a comment about this in the Ipv4Header documentation ?
> http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode1320
> src/internet/model/ipv4-l3-protocol.cc:1320: // XXX
> is this an XXX, or is it just an observation that this implementation does not
> overwrite or otherwise look for fragmentation attacks?
XXX is a refuso. The comment is about a mismatch between this implementation and fragmentation attack vulnerable IP stacks (as is most of IP stacks). The difference arises from the actual implementation, as in kernels it's used a circular buffer where the newer fragment overwrite the oldest one.
On the other hand implementing that in ns-3 is not necessary imho and it would make the implementation way more complex. The packet would had to be reunited on the fly when a fragment arrives and the newer fragment data would had to overwrite the packet's content. Hellish and unnecessary.
Again, a matter of documenting the fact that this implementation does not allow those kind of tricks.
Maybe more a matter of doxygen than anything else.
Cheers,
T.
Message from tommypec@gmail.com
2011-04-23T00:52:31+00:00Tommaso Pecorellaurn:md5:8aa9e18dd497e0f7956bb4e2a2ebcdaa
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc
File src/internet/model/ipv4-l3-protocol.cc (right):
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode738
src/internet/model/ipv4-l3-protocol.cc:738: uint32_t fragmentSize = outInterface->GetDevice ()->GetMtu () & ~uint32_t (0x7);
On 2011/04/22 21:29:49, Tom Henderson wrote:
> suggest a comment on the above cryptic line
Removed the line, unnecessary after some changes.
Done.
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode1153
src/internet/model/ipv4-l3-protocol.cc:1153: // a much more complex handling is necessary in case there are options.
On 2011/04/22 21:29:49, Tom Henderson wrote:
> can you check for options here, and assert if present? (and request in the
> comment for someone to provide a patch to support it)
Can't do any assert as Options are not in ns3 (can't check if UDP or TCP header are present as the user might want to use raw IP).
> playing with IPv4 options is not outside the realm of possibility for a research
> simulator, so we shouldn't lull the user into believing that it is supported.
A comment in the doxygen has been made.
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-protocol.cc#newcode1320
src/internet/model/ipv4-l3-protocol.cc:1320: // XXX
On 2011/04/22 21:29:49, Tom Henderson wrote:
> is this an XXX, or is it just an observation that this implementation does not
> overwrite or otherwise look for fragmentation attacks?
Worked out
Message from tommypec@gmail.com
2011-04-24T13:56:13+00:00Tommaso Pecorellaurn:md5:c1a713dfbf507356df0e1e3bded5c38e
Message from tommypec@gmail.com
2011-04-24T14:16:05+00:00Tommaso Pecorellaurn:md5:77e9c48efb3dc88caa16ca9cd9979120
New code (ironed out in some parts) and new bugs (found and killed ofc).
I had to patch ipv4-header (again) due to a bug. This time is not a matter of API misinterpretation, it's a real one. I love zero-day bugs hiding for centuries in never-used code bits.
The attached patch is just one way to fix it, a deeper discussion is in:
http://www.nsnam.org/bugzilla/show_bug.cgi?id=1102
The patch is included here because anything longer than 8000 bytes will go boom without it.
About the unit test case... I know it's a bit complex, but I haven't found any decent way to drop fragments other than to re-define a channel.
Anyway, the test "just" send 5 packets with increased size (1000, 2000, 5000, 10000 and 65000), then it checks if they're received correctly in the following cases:
1) normal channel
2) reordering channel (fragments out-of-order)
Then the test is repeated dropping some fragments. In this case the test checks if the ICMP::TIME_EXCEEDED is received. Note that the last test works only if the first 8 bytes of the original packet are received (i.e., the first fragment), otherwise no ICMP is sent. I didn't check THAT, but... hell, should be enough for a test, isn't it ?
Last comment: receiving an ICMP is all but obvious, it took me 2 hours to figure out how to do it (it was rather simple at the end, but way counterintuitive).
Plus it seems that the ICMP data are stripped out, and that's not that good. Didn't check deeper into this tho.
It could be a nice idea to define an IcmpSocket (dunno if Linux kernel does that) or, at least, to mention in the UdpSocket documentation how to do it.
Cheers,
T.
Message from tommypec@gmail.com
2011-04-29T20:05:29+00:00Tommaso Pecorellaurn:md5:3df94569fe69c2f00832fb04937cdd0e
I found something I forgot.
When a packet is dropped due to timeout on fragments no trace is fired. That's bad.
Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);" (incorrectly because it's fired for each fragment received but the last, even if it's a perfectly valid fragment an no timeout is exceeded. IPv6 fragmentation requires some fine-tuning.
For IPv4 I'd like to have some cleaner solution, so I'd fire a m_dropTrace only if the fragments are discarded due to timeout.
Problems:
1) How many: one call (it's a single packet anyway) or one per fragment ?
[insert your opinion here]
2) Reason.
In the header the following drop reasons are defined:
enum DropReason
{
DROP_TTL_EXPIRED = 1, /**< Packet TTL has expired */
DROP_NO_ROUTE, /**< No route to host */
DROP_BAD_CHECKSUM, /**< Bad checksum */
DROP_INTERFACE_DOWN, /**< Interface is down so can not send packet */
DROP_ROUTE_ERROR, /**< Route error */
};
None of them seems the right one. I'd add one but I'll have to modify IPv4flowprobe as well.
Objections on defining a new DROP_FRAGMENT_TIMEOUT and add it to flowprobe as well ?
Cheers,
T.
Message from tomh@tomh.org
2011-05-02T04:24:23+00:00tomh_tomh.orgurn:md5:ebd69e11d3990d309f289d0f62dad9f8
On 04/29/2011 01:05 PM, tommypec@gmail.com wrote:
> I found something I forgot.
>
> When a packet is dropped due to timeout on fragments no trace is fired.
> That's bad.
>
> Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);"
> (incorrectly because it's fired for each fragment received but the last,
> even if it's a perfectly valid fragment an no timeout is exceeded. IPv6
> fragmentation requires some fine-tuning.
Please file this bug.
>
> For IPv4 I'd like to have some cleaner solution, so I'd fire a
> m_dropTrace only if the fragments are discarded due to timeout.
> Problems:
> 1) How many: one call (it's a single packet anyway) or one per fragment
> ?
>
I suggest one call per packet.
>
> 2) Reason.
>
> In the header the following drop reasons are defined:
>
> enum DropReason
> {
> DROP_TTL_EXPIRED = 1, /**< Packet TTL has expired */
> DROP_NO_ROUTE, /**< No route to host */
> DROP_BAD_CHECKSUM, /**< Bad checksum */
> DROP_INTERFACE_DOWN, /**< Interface is down so can not send packet
> */
> DROP_ROUTE_ERROR, /**< Route error */
> };
>
> None of them seems the right one. I'd add one but I'll have to modify
> IPv4flowprobe as well.
>
> Objections on defining a new DROP_FRAGMENT_TIMEOUT and add it to
> flowprobe as well ?
>
None from me.
- Tom
Message from tommypec@gmail.com
2011-05-02T10:15:48+00:00Tommaso Pecorellaurn:md5:6f1c729f24b38bf12bd2e2630d5e548d
Yo Tom,
On 02/mag/2011, at 06.24, Tom Henderson wrote:
> On 04/29/2011 01:05 PM, tommypec@gmail.com wrote:
>> I found something I forgot.
>>
>> When a packet is dropped due to timeout on fragments no trace is fired.
>> That's bad.
>>
>> Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);"
>> (incorrectly because it's fired for each fragment received but the last,
>> even if it's a perfectly valid fragment an no timeout is exceeded. IPv6
>> fragmentation requires some fine-tuning.
>
> Please file this bug.
Again ?
https://www.nsnam.org/bugzilla/show_bug.cgi?id=1100
Cheers,
T.
Message from tommypec@gmail.com
2011-05-03T00:16:25+00:00Tommaso Pecorellaurn:md5:f7e54a9bf2dd23a955e6511be881c3df
Message from tomh.org@gmail.com
2011-05-03T04:31:51+00:00Tom Hendersonurn:md5:5da0d84cd3deca7a9ca5902ac6cb8d04
On 2011/05/02 10:15:48, Tommaso Pecorella wrote:
> Yo Tom,
>
>
> On 02/mag/2011, at 06.24, Tom Henderson wrote:
>
> > On 04/29/2011 01:05 PM, mailto:tommypec@gmail.com wrote:
> >> I found something I forgot.
> >>
> >> When a packet is dropped due to timeout on fragments no trace is fired.
> >> That's bad.
> >>
> >> Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);"
> >> (incorrectly because it's fired for each fragment received but the last,
> >> even if it's a perfectly valid fragment an no timeout is exceeded. IPv6
> >> fragmentation requires some fine-tuning.
> >
> > Please file this bug.
>
> Again ?
>
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=1100
>
> Cheers,
>
I should have said fix, not file :)
Sorry about that,
Tom
Message from tommypec@gmail.com
2011-05-03T22:57:44+00:00Tommaso Pecorellaurn:md5:9f8fc4a81c7dcea7cc024b60d8fbece9
Message from tommypec@gmail.com
2011-05-03T23:16:09+00:00Tommaso Pecorellaurn:md5:cf7cce2d4933ce1d23276e86ae835e7f
I know I shouldn't mix things together, but right now I'm a bit wasted from work so I didn't split the patches as they should.
Todo still:
1) split the patches in two, IPv4 and IPv6.
2) make a test case for IPv6 fragmentation.
3) document that some obscure cases of IPv6 fragmentation errors are not handled.
Footnote: why I did all those changes to IPv6. It's the only decent way to handle when and why a packet should be discarded. Plus it should fix the memory leak I found. I have to triple check with valgrind tho. Too sleepy to do it now.
Message from tommypec@gmail.com
2011-05-05T11:39:44+00:00Tommaso Pecorellaurn:md5:8b56721f1d922f53a04f1d32cd66155d
Updated the patch with IPv6 fragmentation fixes
Message from unknown
2011-05-26T07:51:18+00:00Tommaso Pecorellaurn:md5:d148eaf4e8f3b0777f90b40cd09668c1
Message from tommypec@gmail.com
2011-05-26T07:51:27+00:00Tommaso Pecorellaurn:md5:5962baebff1f69d49080761a0db48777
Message from tomh.org@gmail.com
2011-06-13T05:08:45+00:00Tom Hendersonurn:md5:63c0dba34f6a3eb9f30a45d2f4a7a994
sorry for the delay in reviewing. Main comments:
- I am not sure that your proposed change to IPv4 header is what the author intended (despite the bug report 1102)-- will leave comments there
- not sure of the need to introduce ErrorChannel/NetDevice
- not sure of the plan for the file in scratch/
What additional tests should be done? IPv6 fragmentation? Test the ARP cache interaction? Even if we decide to push with just these tests, it would be helpful to document what aspects of fragmentation handling are not being tested.
http://codereview.appspot.com/4440051/diff/26001/scratch/udp-echo-fragm.cc
File scratch/udp-echo-fragm.cc (right):
http://codereview.appspot.com/4440051/diff/26001/scratch/udp-echo-fragm.cc#newcode26
scratch/udp-echo-fragm.cc:26: // - Tracing of queues and packet receptions to file "udp-echo.tr"
Not sure whether this file is intended to be merged. If it is, it needs some cleanup and work on the comments.
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-header.cc
File src/internet/model/ipv4-header.cc (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-header.cc#newcode122
src/internet/model/ipv4-header.cc:122: NS_ASSERT (offset | 0x7);
This change in assert condition is based on changing the semantics of offset...
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-header.h
File src/internet/model/ipv4-header.h (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-header.h#newcode75
src/internet/model/ipv4-header.h:75: * \param offset the ipv4 fragment offset measured in bytes from the start.
it seems to me that the original intention of the API was to keep the units in 8-byte blocks but here you are changing it to bytes.
As a user, I would find this to be surprising. I would expect that I would pass in a maximally 13-bit value here specifying it according to the semantics of the field in the Ipv4 header.
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-header.h#newcode115
src/internet/model/ipv4-header.h:115: * \returns the offset of this fragment measured in bytes from the start.
While I agree that your clarification here based on your code changes, I think users may expect this to be in the actual units of fragmentation offset.
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-header.h#newcode164
src/internet/model/ipv4-header.h:164: uint16_t m_fragmentOffset;
Again, it seems to me here that you are changing the original intent of this code.
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-protocol.cc
File src/internet/model/ipv4-l3-protocol.cc (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-protocol.cc#newcode52
src/internet/model/ipv4-l3-protocol.cc:52: TypeId
lots of whitespace/style changes in this file. what arguments to check-style.py were used (and what version)?
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-protocol.cc#newcode1138
src/internet/model/ipv4-l3-protocol.cc:1138:
these new methods are missing NS_LOG_FUNCTION
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-protocol.h
File src/internet/model/ipv4-l3-protocol.h (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-protocol.h#newcode363
src/internet/model/ipv4-l3-protocol.h:363: * \brief If other fragments will be sent.
"True if other fragmenets will be sent"
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-channel.cc
File src/internet/test/error-channel.cc (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-channel.cc#newcode18
src/internet/test/error-channel.cc:18: * Author: Mathieu Lacage <mathieu.lacage@sophia.inria.fr>
author statement correct here? This is just cloned from simple-channel but maybe add your own author line also since it was entirely rewritten.
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-channel.cc#newcode30
src/internet/test/error-channel.cc:30:
In general, I am not sure about the need to add this set of new classes (ErrorChannel/NetDevice) when we have SimpleNetDevice/Channel to which one can add ErrorModels.
This is just local to a test directory but I am left wondering why you just didn't reuse SimpleChannel.
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-channel.h
File src/internet/test/error-channel.h (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-channel.h#newcode52
src/internet/test/error-channel.h:52:
Doxygen on the following two methods would be useful.
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragmentation-test.cc
File src/internet/test/ipv4-fragmentation-test.cc (right):
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragmentation-test.cc#newcode117
src/internet/test/ipv4-fragmentation-test.cc:117: : TestCase ("Verify the IPv4 layer 3 protocol fragmentation")
"and reassembly"
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragmentation-test.cc#newcode359
src/internet/test/ipv4-fragmentation-test.cc:359: // Each other fragment will arrive out-of-order.
Can you explain a bit more about what you expect to happen in this test case?
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragmentation-test.cc#newcode388
src/internet/test/ipv4-fragmentation-test.cc:388: // Server -> Client : errors disabled (we want to have back the ICMP)
similarly, can you specify for future readers why an ICMP is expected here?
http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragmentation-test.cc#newcode409
src/internet/test/ipv4-fragmentation-test.cc:409: NS_TEST_EXPECT_MSG_EQ ((recvSize == 0), true, "Sever got a packet, something wrong");
Server
Message from tommypec@gmail.com
2011-06-13T10:14:46+00:00Tommaso Pecorellaurn:md5:c0d3f4437ad4a6f44fa325498834282d
On 2011/06/13 05:08:45, Tom Henderson wrote:
> sorry for the delay in reviewing. Main comments:
>
> - I am not sure that your proposed change to IPv4 header is what the author
> intended (despite the bug report 1102)-- will leave comments there
>
> - not sure of the need to introduce ErrorChannel/NetDevice
>
> - not sure of the plan for the file in scratch/
>
> What additional tests should be done? IPv6 fragmentation? Test the ARP cache
> interaction? Even if we decide to push with just these tests, it would be
> helpful to document what aspects of fragmentation handling are not being tested.
Hi Tom,
no problem for the delay, I'm in a rush between two meetings abroad, so I'll be quick.
- IPv4header changes (bug report 1102): I think we'll fix it separately there, this code will "simply" follow.
- ErrorChannel/NetDevice: SimpleChannel doesn't allow packet reordering, which was necessary to test the out-of-order fragments reassembly. Those channels and models are just being used into tests, so I didn't document them fully. I'll do.
- scratch/ file: not intended to be included in the patch. I'll kill it.
- Tests: IPv4 ones are all tested. As for IPv6 is concerned, I didn't modify the actual tests, so I should check them. If I remember fine they don't check:
* out-of-order reassembly
* timeout and ICMP error message
I was keen on implementing also IPv6 fragmentation tests, but I need a way to receive ICMP error messages in the Application code. Since UDPv6 is missing I have some issues in hooking the ICMPv6 receive, also 'cuz ICPMv6 is handled in a way different way than v4 ones.
The ICMP error message is mandatory according to the RFCs, I'll point out in the doxygen and comments.
Hopefully I'll clean out everything and I'll post a new patch next week.
Cheers,
Tommaso
Message from unknown
2011-06-24T00:39:12+00:00Tommaso Pecorellaurn:md5:b383b5121486740cfeb4bfda0ec9c770
Message from tommypec@gmail.com
2011-06-24T00:39:14+00:00Tommaso Pecorellaurn:md5:2e837a971c4952c374e558cfa24a07f7
Message from tommypec@gmail.com
2011-06-24T00:44:29+00:00Tommaso Pecorellaurn:md5:b2c5bdad3ac9410281630a06b67dfbb5
This is only to cleanup the comments and remove the unnecessary files (scratch and IPv4header.cc/.h).
I have to check if I can scrap out also the ErrorNetDevice, maybe yes. The other 2 (channel and model) are necessary as the "Simple" one doesn't do what we need for the test.
About the last "bit", I am eager to have an hint. How can I "trap" an ICMPv6 message ? I mean, I have to have a function called when a particular ICMPv6 type is received, but beat me in the head if I have found a way to do that. v4 have a callback, v6 seems not.
Am I dumb or I missed something ? Or I found another bug ?
Cheers,
Tommaso
Message from rivanvx@gmail.com
2011-06-24T15:51:05+00:00Vedran Miletićurn:md5:3be76071c9075d59223c4ef92a122d17
Sorry for the delay on reviewing.
Implementation looks good to me. Compared to Mathieu's yans implementation I adapted for ns-3, this one uses STL more and looks much cleaner.
API change that Tom mentioned can also be implemented here simply by renaming SendRealOut to SendOut.
A couple of comments on the code are below.
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc
File src/internet/model/ipv4-l3-protocol.cc (right):
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode62
src/internet/model/ipv4-l3-protocol.cc:62: .AddAttribute ("WaitReplyTimeout",
Why is it named WaitReply?
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode1207
src/internet/model/ipv4-l3-protocol.cc:1207: uint32_t idProto = uint32_t (ipHeader.GetIdentification ()) << 16 & uint32_t (ipHeader.GetProtocol ());
Wouldn't it be better to use std::pair here (both this and previous line)?
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode1385
src/internet/model/ipv4-l3-protocol.cc:1385: Ipv4L3Protocol::HandleFragmentsTimeout (std::pair<uint64_t, uint32_t> key, Ipv4Header & ipHeader, uint32_t iif)
I don't understand when does this get called.
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.h
File src/internet/model/ipv4-l3-protocol.h (right):
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.h#newcode370
src/internet/model/ipv4-l3-protocol.h:370: std::list<std::pair<Ptr<Packet>, uint16_t> > m_fragments;
This is really good.
Message from tommypec@gmail.com
2011-06-24T19:49:00+00:00Tommaso Pecorellaurn:md5:1b49cfa933e22b6abe02343293a4d188
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> Sorry for the delay on reviewing.
No problem at all, we was discussing about the IPv4header :)
> Implementation looks good to me. Compared to Mathieu's yans implementation I
> adapted for ns-3, this one uses STL more and looks much cleaner.
Thanks :)
> API change that Tom mentioned can also be implemented here simply by renaming
> SendRealOut to SendOut.
Can you point me to that discussion? I missed it.
> A couple of comments on the code are below.
Replies below.
Cheers,
Tommaso
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc
File src/internet/model/ipv4-l3-protocol.cc (right):
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode62
src/internet/model/ipv4-l3-protocol.cc:62: .AddAttribute ("WaitReplyTimeout",
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> Why is it named WaitReply?
Honestly, I can't remember. The only twisted explanation might be: It's a Timeout. You must wait for it. In case it expires you have to send a Reply (an ICMP error indeed).
Either I copy-pasted from somewhere else and I forgot to rename it :)
Probably I was very tired when I named it. I'll change to something more meaningful. Actually less meaningless...
FragmentExpirationTimeout seems good enough.
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode1207
src/internet/model/ipv4-l3-protocol.cc:1207: uint32_t idProto = uint32_t (ipHeader.GetIdentification ()) << 16 & uint32_t (ipHeader.GetProtocol ());
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> Wouldn't it be better to use std::pair here (both this and previous line)?
Could be, but the key is a Pair already and those are the elements of the key. I didn't want to "over"-use Pairs, as those are perfectly hold by uint64 and uint32.
Using Pair for those as well could slow down the key comparison later, as comparing two uints is way more efficient than recursively comparing pairs.
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode1385
src/internet/model/ipv4-l3-protocol.cc:1385: Ipv4L3Protocol::HandleFragmentsTimeout (std::pair<uint64_t, uint32_t> key, Ipv4Header & ipHeader, uint32_t iif)
In Ipv4L3Protocol::ProcessFragment:
m_fragmentsTimers[key] = Simulator::Schedule (m_waitFragmentsTimeout, &Ipv4L3Protocol::HandleFragmentsTimeout, this, key, ipHeader, iif);
Basically when you receive the first fragment you schedule an event that will fire this function. If you receive all the fragments before that, you cancel the scheduled event.
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.h
File src/internet/model/ipv4-l3-protocol.h (right):
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.h#newcode370
src/internet/model/ipv4-l3-protocol.h:370: std::list<std::pair<Ptr<Packet>, uint16_t> > m_fragments;
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> This is really good.
Thanks :)
Message from unknown
2011-06-24T19:53:39+00:00Tommaso Pecorellaurn:md5:8f0fb13087d70a956f03cdc9bea07ff5
Message from tommypec@gmail.com
2011-06-24T19:53:40+00:00Tommaso Pecorellaurn:md5:3d52d39b0fa71203aab9095e427a3463
Message from rivanvx@gmail.com
2011-06-25T08:21:53+00:00Vedran Miletićurn:md5:2d416306ee21a82300dbae28b509cc79
On 2011/06/24 19:49:00, Tommaso Pecorella wrote:
> > API change that Tom mentioned can also be implemented here simply by renaming
> > SendRealOut to SendOut.
>
> Can you point me to that discussion? I missed it.
>
See Tom's first reply to this.
> http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc
> File src/internet/model/ipv4-l3-protocol.cc (right):
>
> http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode62
> src/internet/model/ipv4-l3-protocol.cc:62: .AddAttribute ("WaitReplyTimeout",
> On 2011/06/24 15:51:05, Vedran Miletić wrote:
> > Why is it named WaitReply?
>
> Honestly, I can't remember. The only twisted explanation might be: It's a
> Timeout. You must wait for it. In case it expires you have to send a Reply (an
> ICMP error indeed).
> Either I copy-pasted from somewhere else and I forgot to rename it :)
>
> Probably I was very tired when I named it. I'll change to something more
> meaningful. Actually less meaningless...
>
> FragmentExpirationTimeout seems good enough.
>
Good.
> http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-protocol.cc#newcode1385
> src/internet/model/ipv4-l3-protocol.cc:1385:
> Ipv4L3Protocol::HandleFragmentsTimeout (std::pair<uint64_t, uint32_t> key,
> Ipv4Header & ipHeader, uint32_t iif)
> In Ipv4L3Protocol::ProcessFragment:
> m_fragmentsTimers[key] = Simulator::Schedule (m_waitFragmentsTimeout,
> &Ipv4L3Protocol::HandleFragmentsTimeout, this, key, ipHeader, iif);
>
> Basically when you receive the first fragment you schedule an event that will
> fire this function. If you receive all the fragments before that, you cancel the
> scheduled event.
>
Thanks, I see now.
Regards,
Vedran
Message from tommypec@gmail.com
2011-06-25T11:41:19+00:00Tommaso Pecorellaurn:md5:737d26523e4af8906e1903ad21c08193
On 2011/06/25 08:21:53, Vedran Miletić wrote:
> On 2011/06/24 19:49:00, Tommaso Pecorella wrote:
> > > API change that Tom mentioned can also be implemented here simply by
> renaming
> > > SendRealOut to SendOut.
> >
> > Can you point me to that discussion? I missed it.
> >
>
> See Tom's first reply to this.
>
Ah, yes. I do remember now.
Well, I didn't change those at all (the APIs I mean) so they're the "old" ones already in the actual implementation.
Whatever the SendRealOut should be renamed SendOut or not is, in my opinion, purely cosmetic as it's a private function.
I guess that Tom was referring to the fact that your approach changed that API slightly by introducing another "Send" function, while here it is preserved the old one.
Cheers,
Tommaso
Message from tommypec@gmail.com
2011-06-28T06:53:43+00:00Tommaso Pecorellaurn:md5:eafa6221960bcf9d7df586032ce8d727
Let's not loose the spin... we're close to finalize this small but quite useful patch.
Vedran: missing comment on SendOut / SendRealOut.
Tom: need more comments (if any).
Myself: need to re-check with uncrustify (was using 0.56, now using 0.58).
Plus: need to document in the tests comments what's being tested and what is NOT being tested (IPv6 in particular).
Side note: I'm not very happy about how ICMPv6L4 protocol is handling ICMPs. I'll propose a separate patch.
Sorry (again) if I'm pushing, but if we loose the spin we'll never end this fix.
Cheers,
Tommaso
Message from tomh.org@gmail.com
2011-06-28T17:45:56+00:00Tom Hendersonurn:md5:6ef62c5177d9ee7076f5c69191aff9b1
Regarding uncrustify, you have a lot of trailing whitespace changes in ipv4-l3-protocol.cc which I suggest that you avoid. We did run uncrustify 0.58 on this before (but not the level 3 which handles trailing whitespace).
My only lingering comments are that it would be nice to split IPv4 and IPv6 patches when committing, and it would be nice to try to copy the ipv4-fragmentation-test to ipv6-fragmentation-test (not sure whether it is blocked by ICMP problems).
http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-protocol.cc
File src/internet/model/ipv4-l3-protocol.cc (right):
http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-protocol.cc#newcode1137
src/internet/model/ipv4-l3-protocol.cc:1137: // Of course also the reassemby code shall be changed as well.
s/reassemby/reassembly
you could consider to make the program abort if this is entered with an IPv4 header option present
Message from tommypec@gmail.com
2011-06-28T18:50:21+00:00Tommaso Pecorellaurn:md5:82f56fa4bf8afa2c36f1359388b578e1
On 2011/06/28 17:45:56, Tom Henderson wrote:
> Regarding uncrustify, you have a lot of trailing whitespace changes in
> ipv4-l3-protocol.cc which I suggest that you avoid. We did run uncrustify 0.58
> on this before (but not the level 3 which handles trailing whitespace).
Doh! That's why. I'll make sure to use lvl 2.
> My only lingering comments are that it would be nice to split IPv4 and IPv6
> patches when committing
I'll do.
> and it would be nice to try to copy the
> ipv4-fragmentation-test to ipv6-fragmentation-test (not sure whether it is
> blocked by ICMP problems).
It is. I'm blocked on this point also for another piece of code (not related to this), and it seems that the ICMPv6L4 proto doesn't have any possible callback to inform that an ICMP has been received. Plus the IPv4 test program does use UDP, not available for v6. I'm not keen blocking the v6 patch 'til those two issues are fixed to be honest, but for sure we can improve the test once they are.
> http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-protocol.cc
> File src/internet/model/ipv4-l3-protocol.cc (right):
>
> http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-protocol.cc#newcode1137
> src/internet/model/ipv4-l3-protocol.cc:1137: // Of course also the reassemby
> code shall be changed as well.
> s/reassemby/reassembly
>
> you could consider to make the program abort if this is entered with an IPv4
> header option present
If only I could test for something that's not here... I mean, there's not even the class to hold IPv4 optional headers in ns-3... testing against them would mean to define them somewhere, and if I'd define them I could handle them as well ! It's a cat trying to catch his own tail.
WHat about a warning in the IPv4 doxygen so any (future) implementor will know that he/she have to check the fragmentation as well ?
Cheers,
Tommaso
Message from tomh@tomh.org
2011-06-28T18:57:18+00:00tomh_tomh.orgurn:md5:9bab9b8ee0ca7282f2b73f297097a08e
On 06/28/2011 11:50 AM, tommypec@gmail.com wrote:
> On 2011/06/28 17:45:56, Tom Henderson wrote:
>> Regarding uncrustify, you have a lot of trailing whitespace changes in
>> ipv4-l3-protocol.cc which I suggest that you avoid. We did run
> uncrustify 0.58
>> on this before (but not the level 3 which handles trailing
> whitespace).
>
> Doh! That's why. I'll make sure to use lvl 2.
>
>> My only lingering comments are that it would be nice to split IPv4 and
> IPv6
>> patches when committing
>
> I'll do.
>
>> and it would be nice to try to copy the
>> ipv4-fragmentation-test to ipv6-fragmentation-test (not sure whether
> it is
>> blocked by ICMP problems).
>
> It is. I'm blocked on this point also for another piece of code (not
> related to this), and it seems that the ICMPv6L4 proto doesn't have any
> possible callback to inform that an ICMP has been received. Plus the
> IPv4 test program does use UDP, not available for v6. I'm not keen
> blocking the v6 patch 'til those two issues are fixed to be honest, but
> for sure we can improve the test once they are.
OK, I agree not to block on this.
>
>
> http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-protocol.cc
>
>> File src/internet/model/ipv4-l3-protocol.cc (right):
>
>
> http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-protocol.cc#newcode1137
>
>> src/internet/model/ipv4-l3-protocol.cc:1137: // Of course also the
> reassemby
>> code shall be changed as well.
>> s/reassemby/reassembly
>
>> you could consider to make the program abort if this is entered with
> an IPv4
>> header option present
>
> If only I could test for something that's not here... I mean, there's
> not even the class to hold IPv4 optional headers in ns-3... testing
> against them would mean to define them somewhere, and if I'd define them
> I could handle them as well ! It's a cat trying to catch his own tail.
i meant trying to determine whether options are present by looking at
the internet header length field.
Message from tommypec@gmail.com
2011-06-28T19:30:08+00:00Tommaso Pecorellaurn:md5:a83b9341e1d7eb1dc5474c50e72798a4
On 2011/06/28 18:57:18, tomh_tomh.org wrote:
> >> you could consider to make the program abort if this is entered with
> > an IPv4
> >> header option present
> >
> > If only I could test for something that's not here... I mean, there's
> > not even the class to hold IPv4 optional headers in ns-3... testing
> > against them would mean to define them somewhere, and if I'd define them
> > I could handle them as well ! It's a cat trying to catch his own tail.
>
> i meant trying to determine whether options are present by looking at
> the internet header length field.
I should not write replies when I'm tired or when I just ate mah dinner. In the latter case (it's 21:30 here) all the blood is working for the stomach and the brain is shut down. You're damn right.
I'm gonna implement it. I'll post a revised patch with this and the fixed uncrustify.
Cheers,
Tommaso
Message from unknown
2011-06-28T20:59:36+00:00Tommaso Pecorellaurn:md5:2acb3d3814103e87c1ce8cf99bd2553c
Message from tommypec@gmail.com
2011-06-28T20:59:48+00:00Tommaso Pecorellaurn:md5:dbe1eb4a871ebaa9e60274b3b47f79dd
Message from tommypec@gmail.com
2011-06-28T21:04:15+00:00Tommaso Pecorellaurn:md5:c275c860c62fc354a662da36e8adfd73
On 2011/06/28 20:59:48, Tommaso Pecorella wrote:
All done. Plan for the patches is (when everyone will be happy about all):
IPv4 Fragmentation and reassembly:
src/flow-monitor/model/ipv4-flow-probe.cc
src/flow-monitor/model/ipv4-flow-probe.h
src/internet/model/ipv4-l3-protocol.cc
src/internet/model/ipv4-l3-protocol.h
src/internet/test/error-channel.cc
src/internet/test/error-channel.h
src/internet/test/error-net-device.cc
src/internet/test/error-net-device.h
src/internet/test/ipv4-fragmentation-test.cc
src/internet/wscript
IPv6 fragmentation improvements:
src/internet/model/ipv6-extension.cc
src/internet/model/ipv6-extension.h
Cheers,
Tommaso
Message from tommypec@gmail.com
2011-07-05T07:50:19+00:00Tommaso Pecorellaurn:md5:534ef1c83486fd7b3829a120938ff4ec
Hi guys,
shall we do a final round of checks or do you think this (2) patches are ready for prime time ?
Cheers,
Tommaso
Message from rivanvx@gmail.com
2011-07-06T12:08:59+00:00Vedran Miletićurn:md5:b1cccfc91586099e3595b45ecb4e32bb
Hi Tommaso,
on the IPv4 side, I believe this is ready for prime time. As for (cosmetic) renaming SendRealOut to SendOut, I would vote for it; it doesn't change the external API and SendOut would make more sense as a name. The names come from yans' functions send_out (that was doing fragmentation) and send_real_out (that was sending fragments to network). Your implementation basically merges them.
Good work.
Regards,
Vedran
Message from tomh.org@gmail.com
2011-07-08T17:17:44+00:00Tom Hendersonurn:md5:f45e6da7ff46fac6a7a5ae3f54011433
just a few more comments
http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-extension.cc
File src/internet/model/ipv6-extension.cc (right):
http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-extension.cc#newcode375
src/internet/model/ipv6-extension.cc:375: // m_dropTrace (packet);
I didn't understand why you set isDropped = true but delete the droptrace.
http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragmentation-test.cc
File src/internet/test/ipv4-fragmentation-test.cc (right):
http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragmentation-test.cc#newcode167
src/internet/test/ipv4-fragmentation-test.cc:167: // NS_LOG_UNCOND("** HandleReadServer " << *packet << " - " << packet->GetSize () );
delete the above?
http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragmentation-test.cc#newcode267
src/internet/test/ipv4-fragmentation-test.cc:267: // LogComponentEnable("Icmpv4L4Protocol", LOG_LEVEL_ALL);
delete the above
Message from tommypec@gmail.com
2011-07-08T18:49:47+00:00Tommaso Pecorellaurn:md5:85529c267223c8ebee28de44b8ac388f
http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-extension.cc
File src/internet/model/ipv6-extension.cc (right):
http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-extension.cc#newcode375
src/internet/model/ipv6-extension.cc:375: // m_dropTrace (packet);
On 2011/07/08 17:17:45, Tom Henderson wrote:
> I didn't understand why you set isDropped = true but delete the droptrace.
The isDropped is set because no packet is forwarded up, but the packet (as a whole) is not dropped, so the trace is not set.
I'll recheck. Maybe it's not necessary to set isDropped to true.
http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragmentation-test.cc
File src/internet/test/ipv4-fragmentation-test.cc (right):
http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragmentation-test.cc#newcode167
src/internet/test/ipv4-fragmentation-test.cc:167: // NS_LOG_UNCOND("** HandleReadServer " << *packet << " - " << packet->GetSize () );
On 2011/07/08 17:17:45, Tom Henderson wrote:
> delete the above?
Aye, forgot.
http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragmentation-test.cc#newcode267
src/internet/test/ipv4-fragmentation-test.cc:267: // LogComponentEnable("Icmpv4L4Protocol", LOG_LEVEL_ALL);
On 2011/07/08 17:17:45, Tom Henderson wrote:
> delete the above
As before, forgot. Will do.
Message from tommypec@gmail.com
2011-07-09T01:20:46+00:00Tommaso Pecorellaurn:md5:f01981374886e8ea34fa37e3602071bc
http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-extension.cc
File src/internet/model/ipv6-extension.cc (right):
http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-extension.cc#newcode375
src/internet/model/ipv6-extension.cc:375: // m_dropTrace (packet);
On 2011/07/08 17:17:45, Tom Henderson wrote:
> I didn't understand why you set isDropped = true but delete the droptrace.
Re-checked. I was right, it have to be set.
In Ipv6L3Protocol::LocalDeliver the "isDropped" is used to warn the L3 that the "packet" (a fragment for real) is on hold and no further actions should be made:
----
nextHeaderPosition += ipv6Extension->Process (p, nextHeaderPosition, ip, dst, &nextHeader, isDropped);
if (isDropped)
{
return;
}
----
Maybe it's a misleading name inside the fragment extension code, but I left the original naming.
The trace, on the contrary, have to be deleted as the fragment is not dropped at all at that time. The trace will be called when and if the whole packet will be dropped along with all the fragments received so far (after the timeout).
So the above code is correct.
Tommaso.
Message from unknown
2011-07-09T01:25:47+00:00Tommaso Pecorellaurn:md5:b2b0c8ae4e37a1e90bd43919b82804bd
Message from tommypec@gmail.com
2011-07-09T01:25:49+00:00Tommaso Pecorellaurn:md5:e0b268b13b5b46d5a1f26927c1b4e493