This is an archive of the discontinued LLVM Phabricator instance.

Split Socket class into Tcp/Udp/DomainSocket subclasses.
ClosedPublic

Authored by ovyalov on Oct 14 2015, 3:59 PM.

Details

Reviewers
zturner
clayborg
Summary

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.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 37411.Oct 14 2015, 3:59 PM
ovyalov retitled this revision from to Split Socket class into Tcp/Udp/DomainSocket subclasses..
ovyalov updated this object.
ovyalov added reviewers: clayborg, zturner, labath.
ovyalov added a subscriber: lldb-commits.
clayborg requested changes to this revision.Oct 14 2015, 4:51 PM
clayborg edited edge metadata.

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.

source/Host/common/Socket.cpp
402

indent wrong

This revision now requires changes to proceed.Oct 14 2015, 4:51 PM
zturner edited edge metadata.Oct 14 2015, 6:27 PM

Can you compile on Windows and make sure it works? Also should you add a unittest for Udp Sockets?

labath accepted this revision.Oct 15 2015, 1:36 AM
labath edited edge metadata.

Looks good, with some random comments you can ignore.

include/lldb/Host/Socket.h
55

I would rename these to something more neutral like name, addr, etc. as this will not be a host+port for the domain socket implementation.

source/Host/common/Socket.cpp
356

Should we make this and Accept() abstract, as all implementations override it anyway?

source/Host/common/TcpSocket.cpp
254 ↗(On Diff #37411)

This comment sounds obsolete. We should probably delete it.

source/Host/posix/DomainSocket.cpp
74

Not really in scope of this patch, but I must say I find blindly deleting a file like this dangerous.

ovyalov updated this revision to Diff 37525.Oct 15 2015, 3:13 PM
ovyalov edited edge metadata.
ovyalov marked 3 inline comments as done.

Addressed review suggestions.
Please take another look.

zturner accepted this revision.Oct 15 2015, 3:18 PM
zturner edited edge metadata.

Looks good.

ovyalov added inline comments.Oct 15 2015, 3:18 PM
source/Host/common/Socket.cpp
356

Good point - done.

source/Host/posix/DomainSocket.cpp
74

If we're going to use unique file names per socket it shouldn't be too dangerous.

labath added inline comments.Oct 16 2015, 1:11 AM
source/Host/posix/DomainSocket.cpp
74

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.

ovyalov added inline comments.Oct 16 2015, 9:21 AM
source/Host/posix/DomainSocket.cpp
74

I was thinking about using abstract sockets but didn't try them in runtime on Android - will check.

labath resigned from this revision.Nov 30 2015, 2:44 AM
labath removed a reviewer: labath.
clayborg accepted this revision.Nov 30 2015, 9:18 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Nov 30 2015, 9:18 AM
ovyalov closed this revision.Dec 2 2015, 11:01 AM

Submitted as r250474