Re: [Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-12-02 Thread Oleksiy Vyalov via lldb-commits 
ovyalov closed this revision.
ovyalov added a comment.

Submitted as r250474


http://reviews.llvm.org/D13754



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-10-16 Thread Pavel Labath via lldb-commits 
labath added inline comments.


Comment at: source/Host/posix/DomainSocket.cpp:74
@@ +73,3 @@
+FileSystem::Unlink(FileSpec{name, true});
+
+Error error;

If we're going to use unique names, then this won't be necessary (and I would 
much rather see a random error opening a socket than a random file 
disappearing). BTW, have you considered using abstract sockets for the 
lldb-server use case? Albeit linux-specific, I find them much nicer, as they 
have no connection to the file system whatsoever.


http://reviews.llvm.org/D13754



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-10-16 Thread Oleksiy Vyalov via lldb-commits 
ovyalov added inline comments.


Comment at: source/Host/posix/DomainSocket.cpp:74
@@ +73,3 @@
+FileSystem::Unlink(FileSpec{name, true});
+
+Error error;

labath wrote:
> If we're going to use unique names, then this won't be necessary (and I would 
> much rather see a random error opening a socket than a random file 
> disappearing). BTW, have you considered using abstract sockets for the 
> lldb-server use case? Albeit linux-specific, I find them much nicer, as they 
> have no connection to the file system whatsoever.
I was thinking about using abstract sockets but didn't try them in runtime on 
Android - will check.



http://reviews.llvm.org/D13754



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-10-15 Thread Oleksiy Vyalov via lldb-commits 
ovyalov updated this revision to Diff 37525.
ovyalov marked 3 inline comments as done.
ovyalov added a comment.

Addressed review suggestions.
Please take another look.


http://reviews.llvm.org/D13754

Files:
  include/lldb/Host/Socket.h
  include/lldb/Host/common/TCPSocket.h
  include/lldb/Host/common/UDPSocket.h
  include/lldb/Host/posix/DomainSocket.h
  lldb.xcodeproj/project.pbxproj
  source/Host/CMakeLists.txt
  source/Host/common/Socket.cpp
  source/Host/common/TCPSocket.cpp
  source/Host/common/UDPSocket.cpp
  source/Host/posix/ConnectionFileDescriptorPosix.cpp
  source/Host/posix/DomainSocket.cpp
  source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
  source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  tools/lldb-server/lldb-platform.cpp
  unittests/Host/SocketTest.cpp

Index: unittests/Host/SocketTest.cpp
===
--- unittests/Host/SocketTest.cpp
+++ unittests/Host/SocketTest.cpp
@@ -13,11 +13,20 @@
 #include 
 #endif
 
+#include 
+#include 
 #include 
 
 #include "gtest/gtest.h"
 
+#include "lldb/Host/Config.h"
 #include "lldb/Host/Socket.h"
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/common/UDPSocket.h"
+
+#ifndef LLDB_DISABLE_POSIX
+#include "lldb/Host/posix/DomainSocket.h"
+#endif
 
 using namespace lldb_private;
 
@@ -46,45 +55,40 @@
 AcceptThread(Socket *listen_socket, const char *listen_remote_address, bool child_processes_inherit,
  Socket **accept_socket, Error *error)
 {
-*error = listen_socket->BlockingAccept(listen_remote_address, child_processes_inherit, *accept_socket);
+*error = listen_socket->Accept(listen_remote_address, child_processes_inherit, *accept_socket);
 }
 
+template
 void
-CreateConnectedSockets(std::unique_ptr *a_up, std::unique_ptr *b_up)
+CreateConnectedSockets(const char *listen_remote_address, const std::function _connect_addr, std::unique_ptr *a_up, std::unique_ptr *b_up)
 {
-Predicate port_predicate;
-// Used when binding to port zero to wait for the thread
-// that creates the socket, binds and listens to resolve
-// the port number.
-
-port_predicate.SetValue(0, eBroadcastNever);
-
 bool child_processes_inherit = false;
-Socket *socket = nullptr;
-const char *listen_remote_address = "localhost:0";
-Error error = Socket::TcpListen(listen_remote_address, child_processes_inherit, socket, _predicate);
-std::unique_ptr listen_socket_up(socket);
-socket = nullptr;
+Error error;
+std::unique_ptr listen_socket_up(new SocketType(child_processes_inherit, error));
+EXPECT_FALSE(error.Fail());
+error = listen_socket_up->Listen(listen_remote_address, 5);
 EXPECT_FALSE(error.Fail());
-EXPECT_NE(nullptr, listen_socket_up.get());
 EXPECT_TRUE(listen_socket_up->IsValid());
 
 Error accept_error;
 Socket *accept_socket;
 std::thread accept_thread(AcceptThread, listen_socket_up.get(), listen_remote_address, child_processes_inherit,
   _socket, _error);
 
-char connect_remote_address[64];
-snprintf(connect_remote_address, sizeof(connect_remote_address), "localhost:%u", port_predicate.GetValue());
-error = Socket::TcpConnect(connect_remote_address, child_processes_inherit, socket);
-a_up->reset(socket);
-socket = nullptr;
+std::string connect_remote_address = get_connect_addr(*listen_socket_up);
+std::unique_ptr connect_socket_up(new SocketType(child_processes_inherit, error));
+EXPECT_FALSE(error.Fail());
+error = connect_socket_up->Connect(connect_remote_address.c_str());
+EXPECT_FALSE(error.Fail());
+EXPECT_TRUE(connect_socket_up->IsValid());
+
+a_up->swap(connect_socket_up);
 EXPECT_TRUE(error.Success());
 EXPECT_NE(nullptr, a_up->get());
 EXPECT_TRUE((*a_up)->IsValid());
 
 accept_thread.join();
-b_up->reset(accept_socket);
+b_up->reset(static_cast(accept_socket));
 EXPECT_TRUE(accept_error.Success());
 EXPECT_NE(nullptr, b_up->get());
 EXPECT_TRUE((*b_up)->IsValid());
@@ -124,25 +128,74 @@
 EXPECT_STREQ ("0", port_str.c_str ());
 EXPECT_EQ (0, port);
 EXPECT_TRUE (error.Success ());
-
 }
 
-TEST_F (SocketTest, Listen0ConnectAccept)
+#ifndef LLDB_DISABLE_POSIX
+TEST_F (SocketTest, DomainListen0ConnectAccept)
 {
-std::unique_ptr socket_a_up;
-std::unique_ptr socket_b_up;
-CreateConnectedSockets (_a_up, _b_up);
+char* file_name_str = tempnam(nullptr, nullptr);
+EXPECT_NE (nullptr, file_name_str);
+const std::string file_name(file_name_str);
+free(file_name_str);
+
+std::unique_ptr socket_a_up;
+std::unique_ptr socket_b_up;
+

Re: [Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-10-15 Thread Oleksiy Vyalov via lldb-commits 
ovyalov added inline comments.


Comment at: source/Host/common/Socket.cpp:356
@@ -740,3 +355,3 @@
 
-uint16_t Socket::GetLocalPortNumber(const NativeSocket& socket)
+size_t Socket::Send(const void *buf, const size_t num_bytes)
 {

Good point - done.


Comment at: source/Host/posix/DomainSocket.cpp:73
@@ +72,3 @@
+
+FileSystem::Unlink(FileSpec{name, true});
+

labath wrote:
> Not really in scope of this patch, but I must say I find blindly deleting a 
> file like this dangerous.
If we're going to use unique file names per socket it shouldn't be too 
dangerous.


http://reviews.llvm.org/D13754



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-10-14 Thread Oleksiy Vyalov via lldb-commits 
ovyalov created this revision.
ovyalov added reviewers: clayborg, zturner, labath.
ovyalov added a subscriber: lldb-commits.
Herald added subscribers: srhines, danalbert, tberghammer.

There are a few reasons for this change:
 - Support generic logic flow for different socket types - a preparation step 
to support domain sockets in lldb-server along with TCP.
 - Offload Socket.cpp and make it more readable and serving more as a factory 
class.
 - Move out protocol-specific members like m_udp_send_sockaddr to more 
fine-grained classes.

http://reviews.llvm.org/D13754

Files:
  include/lldb/Host/Socket.h
  include/lldb/Host/common/TcpSocket.h
  include/lldb/Host/common/UdpSocket.h
  include/lldb/Host/posix/DomainSocket.h
  lldb.xcodeproj/project.pbxproj
  source/Host/CMakeLists.txt
  source/Host/common/Socket.cpp
  source/Host/common/TcpSocket.cpp
  source/Host/common/UdpSocket.cpp
  source/Host/posix/ConnectionFileDescriptorPosix.cpp
  source/Host/posix/DomainSocket.cpp
  source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
  source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  tools/lldb-server/lldb-platform.cpp
  unittests/Host/SocketTest.cpp

Index: unittests/Host/SocketTest.cpp
===
--- unittests/Host/SocketTest.cpp
+++ unittests/Host/SocketTest.cpp
@@ -18,6 +18,7 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Host/Socket.h"
+#include "lldb/Host/common/TcpSocket.h"
 
 using namespace lldb_private;
 
@@ -46,7 +47,7 @@
 AcceptThread(Socket *listen_socket, const char *listen_remote_address, bool child_processes_inherit,
  Socket **accept_socket, Error *error)
 {
-*error = listen_socket->BlockingAccept(listen_remote_address, child_processes_inherit, *accept_socket);
+*error = listen_socket->Accept(listen_remote_address, child_processes_inherit, *accept_socket);
 }
 
 void
@@ -139,10 +140,13 @@
 std::unique_ptr socket_a_up;
 std::unique_ptr socket_b_up;
 CreateConnectedSockets (_a_up, _b_up);
-
-EXPECT_EQ (socket_a_up->GetLocalPortNumber (), socket_b_up->GetRemotePortNumber ());
-EXPECT_EQ (socket_b_up->GetLocalPortNumber (), socket_a_up->GetRemotePortNumber ());
-EXPECT_NE (socket_a_up->GetLocalPortNumber (), socket_b_up->GetLocalPortNumber ());
-EXPECT_STREQ ("127.0.0.1", socket_a_up->GetRemoteIPAddress ().c_str ());
-EXPECT_STREQ ("127.0.0.1", socket_b_up->GetRemoteIPAddress ().c_str ());
+
+TcpSocket* tcp_socket_a(static_cast(socket_a_up.get()));
+TcpSocket* tcp_socket_b(static_cast(socket_b_up.get()));
+
+EXPECT_EQ (tcp_socket_a->GetLocalPortNumber (), tcp_socket_b->GetRemotePortNumber ());
+EXPECT_EQ (tcp_socket_b->GetLocalPortNumber (), tcp_socket_a->GetRemotePortNumber ());
+EXPECT_NE (tcp_socket_a->GetLocalPortNumber (), tcp_socket_b->GetLocalPortNumber ());
+EXPECT_STREQ ("127.0.0.1", tcp_socket_a->GetRemoteIPAddress ().c_str ());
+EXPECT_STREQ ("127.0.0.1", tcp_socket_b->GetRemoteIPAddress ().c_str ());
 }
Index: tools/lldb-server/lldb-platform.cpp
===
--- tools/lldb-server/lldb-platform.cpp
+++ tools/lldb-server/lldb-platform.cpp
@@ -29,7 +29,7 @@
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostGetOpt.h"
 #include "lldb/Host/OptionParser.h"
-#include "lldb/Host/Socket.h"
+#include "lldb/Host/common/TcpSocket.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "LLDBServerUtilities.h"
@@ -283,22 +283,26 @@
 exit(option_error);
 }
 
-std::unique_ptr listening_socket_up;
 Socket *socket = nullptr;
 const bool children_inherit_listen_socket = false;
 
 // the test suite makes many connections in parallel, let's not miss any.
 // The highest this should get reasonably is a function of the number 
 // of target CPUs.  For now, let's just use 100
 const int backlog = 100;
-error = Socket::TcpListen(listen_host_port.c_str(), children_inherit_listen_socket, socket, NULL, backlog);
+std::unique_ptr listening_socket_up(new TcpSocket(children_inherit_listen_socket, error));
+if (error.Fail())
+{
+fprintf(stderr, "failed to create socket: %s", error.AsCString());
+exit(socket_error);
+}
+
+error = listening_socket_up->Listen(listen_host_port.c_str(), backlog);
 if (error.Fail())
 {
 printf("error: %s\n", error.AsCString());
 exit(socket_error);
 }
-listening_socket_up.reset(socket);
-printf ("Listening for a connection from %u...\n", listening_socket_up->GetLocalPortNumber());
 if (port_file)
 {
 error = save_port_to_file(listening_socket_up->GetLocalPortNumber(), port_file);
@@ -322,7 +326,7 @@
 
 const bool children_inherit_accept_socket = true;
 socket = nullptr;
-error =

Re: [Lldb-commits] [PATCH] D13754: Split Socket class into Tcp/Udp/DomainSocket subclasses.

2015-10-14 Thread Greg Clayton via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Can we change "TcpSocket" to be "TCPSocket" and "UdpSocket" to be "UDPSocket" 
in all code and in the file names? Other than that it looks good.



Comment at: source/Host/common/Socket.cpp:403
@@ +402,3 @@
+socketType |= SOCK_CLOEXEC;
+  #endif
+auto sock = ::socket (domain, socketType, protocol);

indent wrong


http://reviews.llvm.org/D13754



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits