Re: Review Request: This change reworks the socket level IO code for the C++ qpid code

[Gordon Sim <gs...@redhat.com>]

> On Aug. 31, 2012, 6:07 a.m., Gordon Sim wrote:
> > Looks good to me, and gets rid of a lot of duplicated code and unnecessary complexity from what I can tell from a relatvely brief read through. So now, to support a new 'transport' for an existing poller (on the server side at least) you only need a new socket implementation, and the protocol factory simply creates the listening sokcet of the correct type and starts listening on it? Would be nice to have a few lines describing the design somewhere.
> 
> Gordon Sim wrote:
>     One other thought: I don't think we have any federation with ssl/rdma tests as part of make check (or indeed *any* tests for rdma).
> 
> Andrew Stitcher wrote:
>     I think your description of the Socket implementation discriminating between the different "transports" using standard poll/read/write semantics is correct. However the RDMA implementation is essentially unchanged from before (except to use the new IOHandle) as it is a very different implementation being entirely async.
>     
>     Where do you think would be a good place to describe the design?

Ok, so really it is that there is one transport, the posix AsynchIO transport, that is itself pluggable through different implementations of the socket abstraction. However alternate transports can exist as well. I'd say perhaps the header of AsynchIOHandler or such would be a good place for a little bit of description on the design.


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6839/#review10932
-----------------------------------------------------------


On Aug. 31, 2012, 3:21 p.m., Andrew Stitcher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6839/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2012, 3:21 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Steve Huston, and Cliff Jansen.
> 
> 
> Description
> -------
> 
> This is a rework of the qpid C++ code socket level code to remove much (but not yet all) of the duplicated code between different "transports".
> 
> Along the way it allows the Unix SSL code to use IPv6.
> 
> The change is quite large if looked at as a single change; A progression of smaller refactorings to the same effect can be seen in my github repo - this may be easier to digest.
> 
> https://github.com/astitcher/qpid/commits/ssl-converge/
> 
> This is not exactly the end of the road for changes to this code, but IMO it is an improvement so should go in even without being perfect. [Specifically there is still work to do to unify the TCPIOPlugin/SSLPlugin code and TCPConnector/SSLConnector code where there is still a lot of duplication.]
> 
> 
> This addresses bugs QPID-3883 and QPID-4272.
>     https://issues.apache.org/jira/browse/QPID-3883
>     https://issues.apache.org/jira/browse/QPID-4272
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/include/qpid/sys/IOHandle.h 1378663 
>   /trunk/qpid/cpp/include/qpid/sys/posix/PrivatePosix.h 1378663 
>   /trunk/qpid/cpp/src/CMakeLists.txt 1378663 
>   /trunk/qpid/cpp/src/Makefile.am 1378663 
>   /trunk/qpid/cpp/src/qpid/broker/windows/SslProtocolFactory.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/client/TCPConnector.h 1378663 
>   /trunk/qpid/cpp/src/qpid/client/TCPConnector.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIO.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/SecuritySettings.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/Socket.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/SocketAddress.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/SslPlugin.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/TCPIOPlugin.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/epoll/EpollPoller.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/posix/AsynchIO.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/posix/BSDSocket.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/sys/posix/BSDSocket.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/sys/posix/IOHandle.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/posix/PollableCondition.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/posix/PosixPoller.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/posix/Socket.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/posix/SocketAddress.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslHandler.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslIo.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/ssl/SslSocket.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/IOHandle.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/IoHandlePrivate.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/IocpPoller.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/PollableCondition.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/Socket.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.h 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/SslAsynchIO.cpp 1378663 
>   /trunk/qpid/cpp/src/qpid/sys/windows/WinSocket.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/sys/windows/WinSocket.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/ssl.cmake 1378663 
>   /trunk/qpid/cpp/src/ssl.mk 1378663 
>   /trunk/qpid/cpp/src/tests/DispatcherTest.cpp 1378663 
>   /trunk/qpid/cpp/src/tests/PollerTest.cpp 1378663 
> 
> Diff: https://reviews.apache.org/r/6839/diff/
> 
> 
> Testing
> -------
> 
> Passes regular "make check", no specific ssl over IPv6 test added to code base yet, though modifying the existing ssl_test to only use IPv6 works.
> 
> 
> Thanks,
> 
> Andrew Stitcher
> 
>