Skip to content
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

Fixing 2176 #2177

Merged
merged 5 commits into from Jul 2, 2016

Conversation

Projects
None yet
2 participants
@biddisco
Copy link
Contributor

commented May 23, 2016

These patches solve the problem of launching the test correctly, but do not address the memory access violations that occur. These could be merged in, independently of the greater fix.

I do not know how to progress with the sanitizer errors and double deletions mentioned in #2176

@biddisco biddisco force-pushed the fixing_2176 branch 2 times, most recently from 2be9b06 to b315275 May 23, 2016

@hkaiser hkaiser added this to the 0.9.12 milestone May 23, 2016

@hkaiser

This comment has been minimized.

Copy link
Member

commented May 23, 2016

I'd have preferred to have a proper fix for the ipv6 address comparison instead of just removing them. Could you please try to explain what's wrong and perhaps what would have to be done in order to properly fix this?

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

The ipv6 comparison is actually just comparing strings. the correct fix would be to convert them back into ipv6 addresses and compare the binary versions. I implemented this, but the fix was specific for ipv6 and would fail for ipv4, so we need to check what type of addresses they are first and convert them accordingly. Since the underlying address objects that provided the strings must be available somewhere, I decided not to spend time on this and just comment the checks out. As you mentioned, they have never triggered until now and are not serving any useful purpose.

@biddisco biddisco force-pushed the fixing_2176 branch from b315275 to 6f5f5bd May 24, 2016

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2016

@hkaiser I have added a cleanup_ip_address function that takes a string containing an ipv4 or ipv6 address and attempts to convert it into a "standardized" address string. It's a bit of a waste of CPU, but it is only called in Debug mode. I hope that addresses your concerns.

@@ -172,6 +172,27 @@ namespace hpx { namespace util
return true;
}

///////////////////////////////////////////////////////////////////////
// Take an ip v4 or v6 address and "standardize" it for comparison checks
HPX_API_EXPORT std::string cleanup_ip_address(const std::string &addr)

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 24, 2016

Member

Export attributes are needed on the first declaration only. gcc will issue warnings for repeating it on the definition.

if (s>0) break;
}
if (i==2) {
throw std::runtime_error("Invalid IP address string");

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 24, 2016

Member

Please throw exceptions using HPX_THROW_EXCEPTION()

@hkaiser

This comment has been minimized.

Copy link
Member

commented May 24, 2016

@biddisco Thanks, this looks fine. I think we shouldn't worry about wasting cycles in Debug code.

@biddisco biddisco force-pushed the fixing_2176 branch 2 times, most recently from a5bdf99 to f67fe08 May 24, 2016

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2016

@hkaiser I realized that the inet_xxx functions would not compile under windows, so I have added a couple of (untested) snippets I found on stack overflow. I'm on the train and my connection is poor - coulf you trigger a windows build of this and I'll check results later.

@biddisco biddisco force-pushed the fixing_2176 branch 2 times, most recently from a58ee83 to df912ee May 24, 2016

(WSAAddressToString((struct sockaddr *)&ss, sizeof(ss), NULL, dst, &s) == 0)?
dst : NULL;
}
#endif

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 27, 2016

Member

@biddisco These functions are available from ASIO: see boost::asio::detail::socket_ops::inet_pton and boost::asio::detail::socket_ops::inet_ntop.

@biddisco biddisco force-pushed the fixing_2176 branch from df912ee to 968a34f May 29, 2016

@hkaiser

This comment has been minimized.

Copy link
Member

commented Jun 13, 2016

LGTM, thanks!

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

Don't merge this. I think there is a problem with the ip address checks and i have not got around to looking at it again. Once I've double checked it, I'll report back.

Fix the Add_Test cmake macro to correctly handle args
Allow an arbitrary number of 'args' to be passed in and in turn
pass them onto the test runner.

@biddisco biddisco force-pushed the fixing_2176 branch from 98d8d3d to d532773 Jul 2, 2016

biddisco and others added some commits May 23, 2016

Pass the launched_process on the command line to launch_process_test
We use the $<TARGET_FILE:TESTNAME> generator expression to ensure
that if Xcode or some other IDE is used for building, then
the correct path to the launched_process executable is picked up
(i.e. adds Debug/Release etc to the path when necessary)
Add a cleanup_ip_address function to fix ipv6 comparison problem
nb. Use boost:asio routines for inet_ntop inet_pton
Revert to using inet_pton and inet_ntop on linux
Keep boost::asio functions for windows, they do not quite work as
expected on linux - see issue 2177 for details.

@biddisco biddisco force-pushed the fixing_2176 branch from d532773 to 42a95c2 Jul 2, 2016

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2016

I spent some time experimenting and have found that on my mac when I have the VPN to the office open, I get ipv6 addresses that are incorrectly compared by the current code - and this causes the test to fail.
The patches included do a ipv6 cleanup, but there's a catch. When I use the boost::asio::detail::socket_ops::inet_pton and ntop functions - they are too clever and convert the ip address to and from the binary descrition, preserving the %en0 suffix (link-local zone index - I don't fully understand it).
if I use the standard inet_pton and inet_ntop I get this (both resolve to the same)

cleanup 1 fe80:5::bae8:56ff:fe40:c3d0   = fe80:5::bae8:56ff:fe40:c3d0
cleanup 2 fe80::bae8:56ff:fe40:c3d0%en0 = fe80:5::bae8:56ff:fe40:c3d0

but when I use the boost asio functions, I get

cleanup 1 fe80:5::bae8:56ff:fe40:c3d0   = fe80:5::bae8:56ff:fe40:c3d0
cleanup 2 fe80::bae8:56ff:fe40:c3d0%en0 = fe80::bae8:56ff:fe40:c3d0%en0

So I am leaving the linux inet_pton functions in, but using the boost asio functions for windows and have added a comment in the source code pointing to this #issue in the event that anyone ever find a problem on windows with ipv6 addresses and looks at it.

In summary : without these patches, the test fails for me on OSX (when the VPN is used and we have an ipv6 address) . With them it passes.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Jul 2, 2016

Thanks John!

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2016

It says the checks failed, but actually they passed and the build timed out just before finishing.

@hkaiser hkaiser merged commit 224c473 into master Jul 2, 2016

1 check failed

ci/circleci A command timed out during your tests
Details

@hkaiser hkaiser deleted the fixing_2176 branch Jul 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.