astraceroute: stopping when target is reached, finer control on number of packets sent, and other smaller changes #210

uno20001 · 2019-06-25T19:51:11Z

I would like to apologize in advance for the size of this commit. The changes should have been implemented in multiple commits. This was a serious oversight on my part. I also interpreted the 80 character line length limit quite liberally in some places.

The necessary changes have been implemented in order for astraceroute to stop when a response arrives from the target. Furthermore the MAN page has been updated.

Change log:

IPv4 and IPv6 BPF filter have been updated to accept the necessary packets (which enable the program to stop when the target responds)
check_ipv4() and check_ipv6() have been more or less rewritten to make stopping possible
finer control is given to the user on the number of packets sent; a new option: -s, --num-packets have been added to specify the number of packets sent in one probe
most of the panic() messages have been updated to print errno and the actual error message from the system call
PF_* constants have been replaced by AF_* constants where I deemed the change appropriate
the MAN page and help() have been modified to reflect the changes, and extended with extra information for some options
default configuration values are now preprocessor constant

I have tested the changes on multiple machines, with both IPv4 and IPv6, and the program seems to function correctly, however, it is very much possible that bugs have been introduced into the codebase.

This PR should fix issue #204.

The commits have not been squashed and signed off yet, and will not be until the fate of this PR is decided.

tklauser · 2019-06-26T07:56:10Z

Thanks for the PR.

I would like to apologize in advance for the size of this commit. The changes should have been implemented in multiple commits. This was a serious oversight on my part. I also interpreted the 80 character line length limit quite liberally in some places.

Yes, the commit is definitely too large for me to review and touches many (partially unrelated) things which makes it even harder. Please try to split it up into several smaller commits which do just one thing at a time. As for the 80 character line limit, feel free to interpret it liberally. I have done so in the past as well :)

Maybe start with the cleanup changes and then the actual fix for issue #204.

Change log:

IPv4 and IPv6 BPF filter have been updated to accept the necessary packets (which enable the program to stop when the target responds)

check_ipv4() and check_ipv6() have been more or less rewritten to make stopping possible

finer control is given to the user on the number of packets sent; a new option: -s, --num-packets have been added to specify the number of packets sent in one probe

most of the panic() messages have been updated to print errno and the actual error message from the system call

PF_* constants have been replaced by AF_* constants where I deemed the change appropriate

the MAN page and help() have been modified to reflect the changes, and extended with extra information for some options

default configuration values are now preprocessor constant

I think each of these should be more or less one commit with the commit message describing what was changed and why.

I have tested the changes on multiple machines, with both IPv4 and IPv6, and the program seems to function correctly, however, it is very much possible that bugs have been introduced into the code base.

This PR should fix issue #204.

The commits have not been squashed and signed off yet, and will not be until the fate of this PR is decided.

Your work on this is definitely appreciated, but as said above, in the current state it's not reviewable for me in a decent amount of time.

uno20001 · 2019-06-26T13:15:07Z

I have broken up the changes into multiple commits. (It was done the crudest way possible with git, but I am no guru, and it works.) Hopefully now the changes are manageable and can be sufficiently reviewed.

tklauser · 2019-06-26T13:44:46Z

Please rebase your branch on top of current master (i.e. drop the old changes and the "back to current version ..." commit) so it only has the individual changes instead of the big change + revert + individual changes.

uno20001 · 2019-06-26T13:58:43Z

I really hope I did exactly that, and nothing more, nothing less.

tklauser · 2019-06-26T15:09:16Z

Thanks, now it looks better. I'll have a look in detail in the next few days.

tklauser · 2019-06-28T10:57:48Z

Thanks for the update. I didn't yet have time to review this PR thoroughly but noticed some possible improvements regarding the commit format.

Please read through the SubmittingPatches file and follow its instructions regarding the commit format and messages, namely:

for each change provide a a meaningful commit message, which:
. explains the problem the change tries to solve, iow, what is wrong with
the current code without the change.
. justifies the way the change solves the problem, iow, why the result with
the change is better.
add a Signed-off-by line to each commit
prefix the commit message subject with the the changed component followed by, e.g. astraceroute: update packet filters

Please take a look at the git history for a reference of how we expect commit messages to look in general.

uno20001 · 2019-06-28T13:03:34Z

Hopefully now everything is more or less what it should be. I tried to keep the commit messages concise and short, that is why I did not try to justify all my decisions in them. I hope that is not a problem. The commits have been signed off.

uno20001 · 2019-08-08T11:05:10Z

Just pinging to see if there are news about the fate of this PR.

uno20001 · 2019-10-01T21:30:34Z

Could you please inform me about the progress of handling this PR?
@tklauser

tklauser

Some small review comments inline, still didn't get to review all of this. I also cherry-picked the first commit of this PR as 500e708, please drop your change accordingly and rebase on top of current master.

Also, please make sure you set the author name (pseudonym is fine) on each of your commits.

astraceroute.8

tklauser · 2019-10-03T11:56:11Z

astraceroute.c

@@ -303,7 +303,7 @@ static void __noreturn help(void)
 " -6|--ipv6 Use IPv6-only requests\n"
 " -n|--numeric Do not do reverse DNS lookup for hops (default)\n"
 " -u|--update Update GeoIP databases\n"
- " -L|--latitude Show do_geo_lookup and longitude\n"
+ " -L|--latitude Show latitude and longitude\n"


Please squash this with the commit (in your branch) that introduced the typo.

tklauser · 2019-10-03T11:57:37Z

astraceroute.c

+ void (*handler)(uint8_t *packet, size_t len, int do_dns_resolution,
+ int do_geo_lookup);


Change do_dns_resolution and do_geo_lookup to bool type while at it.

uno20001 · 2019-10-08T15:43:43Z

EDIT: I just realized that there is a Reopen and comment button...

My not having a clue about how Git functions really shows here. I tried to rebase onto netsniff-ng:master, Git showed conflicts, and I promptly ignored them doing git rebase --continue. Then I force-pushed to uno20001:master. I hoped that everything would turn out fine, alas, it did not. I noticed (thanks to the automatic testing) that Git inserted some lines (probably to show where rebase failed?). Then, for some reason, I thought it would be a good idea to soft reset my local copy and force-push to uno20001:master and the commit my changes later. Unfortunately the moment I finished force-pushing, GitHub closed the pull request because the branches were the same at that moment. It also did not help that since then I have committed and pushed the changes to uno20001:master. I really hope that this can be solved easily. I do not want to open a new pull request as I think it should be possible to reopen this one somehow.

uno20001 · 2020-10-14T15:29:20Z

@tklauser any updates?

tklauser · 2020-10-19T12:27:30Z

@tklauser any updates?

Sorry @uno20001, this somehow fell between the cracks. I've cherry-picked the first two clean-up commits and will try to get to review the rest of this PR as soon as possible. Thanks.

…in get_remote_fd() Signed-off-by: uno20001 <[email protected]>

…to know when the traceroute target is reached Signed-off-by: uno20001 <[email protected]>

…is reached Signed-off-by: uno20001 <[email protected]>

Signed-off-by: uno20001 <[email protected]>

uno20001 · 2022-06-14T17:31:42Z

I thought I will finally rework this PR into a reasonable form for its three year anniversary. But I have encountered some things that I find a bit confusing.

I would like to modify the main loop to better fit to what is described in the man page:

-q <num>, --num-probes <num>
              Specifies the number of queries to be done on a particular hop. The default is 2  query
              requests.

-x <sec>, --timeout <sec>
              Tells  astraceroute  the  probe response timeout in seconds, in other words the maximum
              time astraceroute must wait for an ICMP response from the current hop. The default is 3
              seconds.

That is, restructure the code so that __probe_remote() does exactly ctx::queries probes (per protocol), and each probe waits until an acceptable reply is received or the timeout elapses, whichever comes first. And the printed time is the median of the successful probes.

@tklauser what do you think?

uno20001 force-pushed the master branch from 88e2ac2 to 16cc28c Compare June 26, 2019 13:58

uno20001 force-pushed the master branch from c5fee8d to 81ba958 Compare June 26, 2019 20:29

uno20001 force-pushed the master branch 2 times, most recently from f302d50 to 1ccd31d Compare June 28, 2019 12:57

tklauser requested changes Oct 3, 2019

View reviewed changes

uno20001 force-pushed the master branch from 1ccd31d to d4ead28 Compare October 8, 2019 09:42

uno20001 closed this Oct 8, 2019

uno20001 force-pushed the master branch from d4ead28 to 5f67447 Compare October 8, 2019 09:49

uno20001 reopened this Oct 8, 2019

uno20001 added 4 commits April 21, 2022 18:06

astraceroute: partially rewrite and make error messages more verbose …

a70a264

…in get_remote_fd() Signed-off-by: uno20001 <[email protected]>

astraceroute: add new IPv4/IPv6 filters that will enable the program …

b43c42e

…to know when the traceroute target is reached Signed-off-by: uno20001 <[email protected]>

astraceroute: fix issue netsniff-ng#204 by detecting when the target …

c79b18e

…is reached Signed-off-by: uno20001 <[email protected]>

astraceroute: add new command line option to help and edit MAN page

db8925c

Signed-off-by: uno20001 <[email protected]>

uno20001 force-pushed the master branch from 1c53e7d to db8925c Compare April 21, 2022 16:07

uno20001 marked this pull request as draft April 21, 2022 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

astraceroute: stopping when target is reached, finer control on number of packets sent, and other smaller changes #210

astraceroute: stopping when target is reached, finer control on number of packets sent, and other smaller changes #210

uno20001 commented Jun 25, 2019

tklauser commented Jun 26, 2019

uno20001 commented Jun 26, 2019

tklauser commented Jun 26, 2019

uno20001 commented Jun 26, 2019

tklauser commented Jun 26, 2019

tklauser commented Jun 28, 2019

uno20001 commented Jun 28, 2019

uno20001 commented Aug 8, 2019

uno20001 commented Oct 1, 2019

tklauser left a comment

tklauser Oct 3, 2019

uno20001 Oct 25, 2019

tklauser Oct 3, 2019

uno20001 Oct 25, 2019

uno20001 commented Oct 8, 2019 •

edited

Loading

uno20001 commented Oct 14, 2020

tklauser commented Oct 19, 2020

uno20001 commented Jun 14, 2022 •

edited

Loading

		void (handler)(uint8_t packet, size_t len, int do_dns_resolution,
		int do_geo_lookup);

astraceroute: stopping when target is reached, finer control on number of packets sent, and other smaller changes #210

Are you sure you want to change the base?

astraceroute: stopping when target is reached, finer control on number of packets sent, and other smaller changes #210

Conversation

uno20001 commented Jun 25, 2019

tklauser commented Jun 26, 2019

uno20001 commented Jun 26, 2019

tklauser commented Jun 26, 2019

uno20001 commented Jun 26, 2019

tklauser commented Jun 26, 2019

tklauser commented Jun 28, 2019

uno20001 commented Jun 28, 2019

uno20001 commented Aug 8, 2019

uno20001 commented Oct 1, 2019

tklauser left a comment

Choose a reason for hiding this comment

tklauser Oct 3, 2019

Choose a reason for hiding this comment

uno20001 Oct 25, 2019

Choose a reason for hiding this comment

tklauser Oct 3, 2019

Choose a reason for hiding this comment

uno20001 Oct 25, 2019

Choose a reason for hiding this comment

uno20001 commented Oct 8, 2019 • edited Loading

uno20001 commented Oct 14, 2020

tklauser commented Oct 19, 2020

uno20001 commented Jun 14, 2022 • edited Loading

uno20001 commented Oct 8, 2019 •

edited

Loading

uno20001 commented Jun 14, 2022 •

edited

Loading