Page MenuHomePhabricator

Add Socket::Get[Remote/Local]IpAddress and unit tests
ClosedPublic

Authored by vharron on Jan 11 2015, 2:22 AM.

Details

Summary

Why did I make this patch?

I am trying to get the remote tests running linux->linux when the hosts are multi-homed, localhost or don't have proper DNS entries.

I need to add Socket::Get[Remote/Local]IpAddress
I wanted to make some unit tests to test it
So I created a unit test binary
Socket has a really, really big number of dependencies (like all of lldb)
I removed references to Args::StringToUInt32 and created SocketGlue to get the unit test binary linking without pulling in everything
It's a bit of a hack but it gets the job done
I needed to use inet_ntop, which doesn't exist on Windows XP, so I implemented that, too

Diff Detail

Repository
rL LLVM

Event Timeline

vharron updated this revision to Diff 17984.Jan 11 2015, 2:22 AM
vharron retitled this revision from to Add Socket::Get[Remote/Local]IpAddress and unit tests.
vharron updated this object.
vharron edited the test plan for this revision. (Show Details)
vharron added reviewers: clayborg, ovyalov, sivachandra.
vharron set the repository for this revision to rL LLVM.
vharron added a subscriber: Unknown Object (MLST).
ovyalov accepted this revision.Jan 12 2015, 10:55 AM
ovyalov edited edge metadata.

Minor comments.

source/Host/common/Socket.cpp
536 ↗(On Diff #17984)

s/65536/UINT16_MAX?

source/Host/common/SocketAddress.cpp
44 ↗(On Diff #17984)

Could you check here for returned null value?

68 ↗(On Diff #17984)

indentation.

This revision is now accepted and ready to land.Jan 12 2015, 10:55 AM
clayborg edited edge metadata.Jan 12 2015, 11:32 AM

The strtoul() calls you added have an issue in that if you type "123xyz" both calls with happily decode 123 as the resulting unsigned long and not error out. Args::StringToUInt32() actually correctly uses in the "endptr" argument and check "*endptr == '\0'" and return fail value which is why we really prefer to not have people use strtoul() manually because they always make these errors. I would prefer to keep using Args::StringToUInt32() if possible so we have one codebase that decodes numbers as strings. I also prefer to not have people remove code like "Args::StringTo*" just because it comes from some other folder in LLDB. LLDB is one large codebase and it needs to stay intact. Please fix the testing code so it can include the other parts of LLDB and reinstate the "#include "lldb/Interpreter/Args.h"" and use the appropriate calls.

clayborg requested changes to this revision.Jan 12 2015, 11:44 AM
clayborg edited edge metadata.

The other option is to move all static Args::StringTo* calls into the Host layer at an appropriate place.

Changes should be:

  • please use Args::StringTo calls instead of manual strtoul() calls so we have a common codebase and so people don't start using strtoul() on their own in other places f the code
  • optionally move the static Args::StringTo* calls into the Host layer
This revision now requires changes to proceed.Jan 12 2015, 11:44 AM
vharron updated this revision to Diff 18264.Jan 15 2015, 2:45 PM
vharron edited edge metadata.
vharron updated this object.
vharron edited the test plan for this revision. (Show Details)

Replace strtoul with ConvertString::ToUInt32

Socket::DecodeHostAndPort was accepting negative port numbers and numbers > 65535, fixed

clayborg requested changes to this revision.Jan 15 2015, 4:28 PM
clayborg edited edge metadata.

You removed a comment from line 1 of source/Host/common/Socket.cpp. Fix that and it will be good to go.

This revision now requires changes to proceed.Jan 15 2015, 4:28 PM
This revision was automatically updated to reflect the committed changes.