Page MenuHomePhabricator

Make UriParser to support [$HOSTNAME] notation.
ClosedPublic

Authored by ovyalov on Aug 13 2015, 8:45 PM.

Details

Summary

UriParser:Parse expects that host name doesn't contain special URL symbols - ":", ";", "$",...
However, in case of adb protocol (adb://$device_serial_no:$port) it poses a problem - device serial number may contain a colon ("192.168.0.1:5555").
In order to handle this situation UriParser now supports optional [$HOSTNAME] notation - i.e. connect://localhost:5555 and connect://[localhost]:5555 should be parsed identically.
This change allows us to pass special URL symbols (like colon) as part of a hostname.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 32125.Aug 13 2015, 8:45 PM
ovyalov retitled this revision from to Make LLDB URLs to support hex encoding for special symbols.
ovyalov updated this object.
ovyalov added a subscriber: lldb-commits.
tberghammer edited edge metadata.Aug 14 2015, 2:12 AM

The implementation looks good, but I don't like the approach you try to handle the problem.

I think you do far too much work to use UriParser in a case where it isn't necessary and isn't make things easier. I think a better (and definitely simpler) solution would be to change only ConnectionFileDescriptorPosix.cpp to parse the adb addresses with a locally specified regexp or with looking for the last colon in the address and split the address to host and port based on that.

The implementation looks good, but I don't like the approach you try to handle the problem.

I think you do far too much work to use UriParser in a case where it isn't necessary and isn't make things easier. I think a better (and definitely simpler) solution would be to change only ConnectionFileDescriptorPosix.cpp to parse the adb addresses with a locally specified regexp or with looking for the last colon in the address and split the address to host and port based on that.

The problem that connection url goes all way down PlatformAndroid::ConnectRemote-> PlatformAndroidRemoteGDBServer::ConnectRemote->PlatformRemoteGDBServer::ConnectRemote whiles it reaches ConnectionFileDescriptorPosix::Connect. Within each ConnectRemote we call UriParser::Parse - either to verify url correctness or fetch device_id from adb url. In this case we may need to have special handling for adb protocol inside of UriParser::Parse to pass it through - I'm not very happy to bring protocol-specific knowledge to this class.

tberghammer accepted this revision.Aug 18 2015, 2:55 AM
tberghammer edited edge metadata.

I see your point. I am still not too happy with this approach but I don't have any better idea to solve it. I am fine with committing it in in this format and we can improve it later if we find a better approach.

This revision is now accepted and ready to land.Aug 18 2015, 2:55 AM
ovyalov updated this revision to Diff 32655.Aug 19 2015, 8:51 PM
ovyalov retitled this revision from Make LLDB URLs to support hex encoding for special symbols to Make UriParser to support [$HOSTNAME] notation..
ovyalov updated this object.
ovyalov edited edge metadata.

Rewrote the CL to make UriParser to support optional [$HOSTNAME] notation for LLDB connect URL - i.e. connect://localhost:5555 and connect://[localhost]:5555 should be parsed identically.
This change allows us to pass special URL symbols (like colon) as part of a hostname.

tberghammer added inline comments.Aug 20 2015, 2:02 AM
source/Utility/UriParser.cpp
13

I think you wanted to include <string>, not <string.h>

52–61

I would prefer no to remove the '[' and the ']' characters from the hostname. At the moment you don't use the hostname for the adb protocol so it isn't matter but if we consider IPv6 then having '[' and ']' around the address sounds like a good idea to me (later stages can use this information).

ovyalov added inline comments.Aug 20 2015, 7:30 AM
source/Utility/UriParser.cpp
13

I need string.h for strlen - it's required even with <string> included .

52–61

Let me leave [] removal as-is since we treat hostname as device_id in case of adb and then use it for port forwarding setup - https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Platform/Android/PlatformAndroid.cpp#L199

labath added a subscriber: labath.Aug 20 2015, 7:37 AM
labath added inline comments.
source/Utility/UriParser.cpp
13

The C++ way would be to #include <cstring>

</driveby>

tberghammer added inline comments.Aug 20 2015, 8:17 AM
source/Utility/UriParser.cpp
13

Sorry, I missed that you use strlen

52–61

I would prefer to remove them in PlatformAndroid when we convert it to devices id, but if you want to leave it this way I am fine with that also.

ovyalov updated this revision to Diff 32701.Aug 20 2015, 9:11 AM
ovyalov marked an inline comment as done.

Replaced #include <string.h> with <cstring>

ovyalov closed this revision.Aug 20 2015, 4:11 PM

Files:

/lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
/lldb/trunk/source/Utility/UriParser.cpp
/lldb/trunk/source/Utility/UriParser.h
/lldb/trunk/unittests/Utility/UriParserTest.cpp

Users:

ovyalov (Author)

http://reviews.llvm.org/rL245639