Open mirabilos opened 2 years ago
mirabilos
commented
2 years ago I’ve added -reverse mode, in which the receiver may be behind NAT and connects to the sender, which waits for a dummy packet (sent by the receiver in a new temporary preconnect thread) and then sends to the IP/port where the dummy packet came from.
This makes it possible to test asymmetric connections that are one-sided behind NAT.
IngJohEricsson
commented
2 years ago Hi Thanks for the hard work. I have a couple of questions : 1) Have you verified that the code works as intended with all the proposed fixes ? 2) Is it possible to put all white space changes as a separate commit?, it would make it easier to review the proposed changes.
Regards /Ingemar
From: mirabilos @.> Sent: Tuesday, 28 June 2022 17:59 To: EricssonResearch/scream @.> Cc: Subscribed @.***> Subject: [EricssonResearch/scream] bwtest NAT support (PR #47)
On top of the changes in #46https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-e8f50fcdfab826f6&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F46 this adds NAT support to the bandwidth tester:
the receiver now only gets its local port to listen to as command line argument and binds to INADDR_ANY on this port
the sender binds to a local ephemeral address and contacts the server by means of its “decoder” IP and port (unchanged)
the receiver gets the first packet and looks at the sender information from recvfrom(2) / recvmsg(2) msg_name and “records” that as outgoing feedback address
any further packets have their sender IP/port checked against the saved address and are verbosely discarded if they don’t match, to avoid mixing different senders in the same receiver program
This has been tested with the receiver on a public IPv4 and the sender behind NAT; it should also work with the sender behind multiple NATs as on Campus-IT PoC.
The totally inefficient and not-working NAT punching from the existing code has been removed as well.
You can view, comment on, or merge this pull request online at:
https://github.com/EricssonResearch/scream/pull/47https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-678eb03b5f281c81&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F47
Commit Summary
File Changes
Patch Links:
— Reply to this email directly, view it on GitHubhttps://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-678eb03b5f281c81&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2FEricssonResearch%2Fscream%2Fpull%2F47, or unsubscribehttps://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-bc00f2435b1430eb&q=1&e=38bac337-83f9-4611-8942-51f581cdd59f&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACRZ2GDLL6XUQ2JB2SXVQ4TVRMONNANCNFSM52CWKCXQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>
mirabilos
commented
2 years ago Hi Ingemar,
1) Have you verified that the code works as intended with all the proposed fixes ?
yes, I’m easily reaching the same bandwidth as without these modifications on an 100 Mbit/s link and 200+ on Gbit links.
2) Is it possible to put all white space changes as a separate commit?, it would make it easier to review the proposed changes.
They are separate commits, but you can also do:
git fetch origin pull/47/head:refs/pr/47 # to fetch the changes
git diff -w master..refs/pr/47 # to view ignoring WS changes
Since this PR is based on top of #46 maybe review+merge that first, then this PR will be reduced to the necessities.
Thanks, //mirabilos -- Infrastrukturexperte • tarent solutions GmbH Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/ Telephon +49 228 54881-393 • Fax: +49 228 54881-235 HRB AG Bonn 5168 • USt-ID (VAT): DE122264941 Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg
IngJohEricsson
commented
1 year ago Hi This one got stuck a little too long. Is it possible that you can do a new pull request based on the latest code ? I understand that the NAT handling stuff is very useful so it would definitely be goo to have it in /Ingemar
On top of the changes in #46 this adds NAT support to the bandwidth tester:
sockaddr_inis initialised withAF_UNSPECto signal it’s not fully initialised yetrecvfrom(2)/recvmsg(2)msg_nameand “records” that as outgoing feedback addressThis has been tested with the receiver on a public IPv4 and the sender behind NAT; it should also work with the sender behind multiple NATs as on Campus-IT PoC.
The totally inefficient and not-working NAT punching from the existing code has been removed as well.