qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.com>
Subject Re: Review Request: This change reworks the socket level IO code for the C++ qpid code
Date Mon, 03 Sep 2012 08:18:22 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message