New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teach portolan traceroute about reply packets. #153

Merged
merged 5 commits into from Oct 14, 2015

Conversation

Projects
None yet
2 participants
@bassosimone
Member

bassosimone commented Aug 29, 2015

This needs some more testing before it can be merged. However I'm opening a pull request to bind it to the upcoming milestone and know this is something to work on.

This has now been refined and moderately tested. Note that this is needed by Portolan.

@bassosimone bassosimone added the bug label Aug 29, 2015

@bassosimone bassosimone self-assigned this Aug 29, 2015

@bassosimone bassosimone added this to the measurement-kit 0.1.0 milestone Aug 29, 2015

@bassosimone bassosimone reopened this Aug 29, 2015

bassosimone added some commits Jul 31, 2015

Test case where remote host sends reply
This test case currently fails as follows:

```
1 192.168.91.17 0.870014 ms
2 130.192.2.65 0.393142 ms
3 130.192.232.60 1.12285 ms
4 130.192.232.254 1.09926 ms
5 193.206.132.33 1.7583 ms
6 90.147.80.217 3.07701 ms
7 90.147.80.73 2.47461 ms
8 213.242.65.81 28.4994 ms
9 *
10 *
11 195.16.160.78 11.2321 ms
12 error: recvmsg() failed
13 error: recvmsg() failed
14 error: recvmsg() failed
15 error: recvmsg() failed
16 error: recvmsg() failed
17 error: recvmsg() failed
18 error: recvmsg() failed
19 error: recvmsg() failed
20 error: recvmsg() failed
21 error: recvmsg() failed
22 error: recvmsg() failed
23 error: recvmsg() failed
24 error: recvmsg() failed
25 error: recvmsg() failed
26 error: recvmsg() failed
27 error: recvmsg() failed
28 error: recvmsg() failed
29 error: recvmsg() failed
30 error: recvmsg() failed
31 error: recvmsg() failed
32 error: recvmsg() failed
33 error: recvmsg() failed
34 error: recvmsg() failed
35 error: recvmsg() failed
36 error: recvmsg() failed
37 error: recvmsg() failed
38 error: recvmsg() failed
39 error: recvmsg() failed
40 error: recvmsg() failed
41 error: recvmsg() failed
42 error: recvmsg() failed
43 error: recvmsg() failed
44 error: recvmsg() failed
45 error: recvmsg() failed
46 error: recvmsg() failed
47 error: recvmsg() failed
48 error: recvmsg() failed
49 error: recvmsg() failed
50 error: recvmsg() failed
51 error: recvmsg() failed
52 error: recvmsg() failed
53 error: recvmsg() failed
54 error: recvmsg() failed
55 error: recvmsg() failed
56 error: recvmsg() failed
57 error: recvmsg() failed
58 error: recvmsg() failed
59 error: recvmsg() failed
60 error: recvmsg() failed
61 error: recvmsg() failed
62 error: recvmsg() failed
63 error: recvmsg() failed
64 error: recvmsg() failed
```
Get responder IP address
After this change the test that earlier failed goes like this:

```
1 192.168.91.17 0.922045 ms
2 130.192.2.65 0.61169 ms
3 130.192.232.60 1.98754 ms
4 130.192.232.254 1.36968 ms
5 193.206.132.33 1.16204 ms
6 90.147.80.217 3.15043 ms
7 90.147.80.73 2.43107 ms
8 213.242.65.81 4.34081 ms
9 *
10 *
11 195.16.160.78 11.2667 ms
12 208.67.222.222 11.0714 ms
```

Hooray!

@bassosimone bassosimone changed the title from Feature/testing portolan opendns to Teach portolan traceroute about reply packets. Oct 7, 2015

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Oct 12, 2015

Member

Hi @ValerioLuconi, can you please review this diff and tell me what you think? I like to merge this so that we can move forward Portolan integration! Thanks.

Member

bassosimone commented Oct 12, 2015

Hi @ValerioLuconi, can you please review this diff and tell me what you think? I like to merge this so that we can move forward Portolan integration! Thanks.

Fix two traceroute/android bugs
1. make sure we properly initialize solen to the size of the
   buffer used to store the address; fixes a bug where the address
   was set to 0.0.0.0 on Linux 4.2.2-1-ARCH.

2. correctly use the length returned by recvfrom rather than the
   maximum size of the buffer for the returned packet payload.

Latter bug spotted while debugging the former.

Committing on behalf of @bassosimone.
@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Oct 14, 2015

Member

@ValerioLuconi confirmed that this diff works as expected from both CNR premises (where it behaves like scamper) and from home. Also he said the diff looked good to him.

Member

bassosimone commented Oct 14, 2015

@ValerioLuconi confirmed that this diff works as expected from both CNR premises (where it behaves like scamper) and from home. Also he said the diff looked good to him.

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Oct 14, 2015

Member

I've checked why regress tests fail. In both cases (clang and gcc) the code fails with error EHOSTUNREACH triggered during sendto(). This error is a transient network error that we can safely ignore (i.e. this error does not tell us that code in this pull request is wrong, it just informs us of a transient network situation). However, this also reminds me that traceroute code should not throw an exception when sendto() fails; rather this error should be routed to on_error() so that the programmer can write the proper handling code. Since this change is not trivial, I will do this in another pull request.

Member

bassosimone commented Oct 14, 2015

I've checked why regress tests fail. In both cases (clang and gcc) the code fails with error EHOSTUNREACH triggered during sendto(). This error is a transient network error that we can safely ignore (i.e. this error does not tell us that code in this pull request is wrong, it just informs us of a transient network situation). However, this also reminds me that traceroute code should not throw an exception when sendto() fails; rather this error should be routed to on_error() so that the programmer can write the proper handling code. Since this change is not trivial, I will do this in another pull request.

bassosimone added a commit that referenced this pull request Oct 14, 2015

Merge pull request #153 from bassosimone/feature/testing-portolan-ope…
…ndns

Teach portolan traceroute about reply packets.

@bassosimone bassosimone merged commit 637dabd into measurement-kit:master Oct 14, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@bassosimone bassosimone deleted the bassosimone:feature/testing-portolan-opendns branch Oct 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment