[Lldb-commits] [PATCH] Refactor Socket into a first class Host-object

[Greg Clayton]

A few comments:
- Any lldb_private forward declarations should be added to "include/lldb/lldb-forward.h" and then that file should be included instead of inlining any forward declarations manually. Also if you are going to use shared pointers to then there is a section in lldb-forward.h at the bottom where you should declare them like the others.

 namespace lldb_private {
 
+class Error;
+class Socket;


- Can we rename IoObject to IOObject? Or is there a better name? IOConnection? IOFile? 

- Add the "_sp" suffix to each of these and use IOObjectSP that you define in lldb-forward.h:

+    std::shared_ptr<IoObject> m_read;
+    std::shared_ptr<IoObject> m_write;

- We have been using the "_ap" suffix for auto pointers, but we are slowly transitioning to using a "_up" for the unique pointers. 

+    std::unique_ptr<Socket> m_listening_socket; // For listen/accept connections, hold onto the

These make me nervous and should probably return a pointer or a shared pointer, not a reference unless they are private and are known to be used only when these are valid:

+    IoObject& GetReadObject() { return *m_read; }
+    const IoObject& GetReadObject() const { return *m_read; }


This kind of code seems like we are trying to make a new class IoObject, but don't want to correctly abstract it down to a virtual class that can do everything that we need of it. Seems like we should add a GetBoundPort(...) to the IoObject class and have it return something invalid instead of casting and having to know what the IoObject is and cast it:

+            if (read_object.GetFdType() == IoObject::eFDTypeSocket)
             {
-                char port_cstr[32];
-                snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i", out_port);
-                // Send the host and port down that debugserver and specify an option
-                // so that it connects back to the port we are listening to in this process
-                debugserver_args.AppendArgument("--reverse-connect");
-                debugserver_args.AppendArgument(port_cstr);
+                Socket& tcp_socket = static_cast<Socket&>(read_object);
+                assert(tcp_socket.GetSocketProtocol() == Socket::ProtocolTcp);
+                out_port = tcp_socket.GetBoundPort(10);

Other than that, lots of code has moved around into the host layer but it is mostly the same. So looks good.

> On Jul 28, 2014, at 4:57 PM, Zachary Turner <zturner at google.com> wrote:
> 
> This fixes the crashing in lldb-gdbserver, and should include all of the necessary things to get it to compile on MacOSX as well.  (I have a request for my own Mac workstation pending, until then I have to cross my fingers and hope I did it right).
> 
> lldb-gdbserver tests still fail, but now it's because of a pipe that's not getting any data.  I suspect that all the remaining test failures now have the same underlying cause.  Still investigating exactly what that might be, though.
> 
> http://reviews.llvm.org/D4641
> 
> Files:
>  include/lldb/Core/ConnectionFileDescriptor.h
>  include/lldb/Host/Editline.h
>  include/lldb/Host/File.h
>  include/lldb/Host/IoObject.h
>  include/lldb/Host/Socket.h
>  lldb.xcodeproj/project.pbxproj
>  source/Core/ConnectionFileDescriptor.cpp
>  source/Host/common/CMakeLists.txt
>  source/Host/common/File.cpp
>  source/Host/common/IoObject.cpp
>  source/Host/common/Socket.cpp
>  source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
>  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
>  tools/lldb-gdbserver/lldb-gdbserver.cpp
>  tools/lldb-platform/lldb-platform.cpp
> <D4641.11964.patch>_______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits