On 10/02/2010 10:06 AM, Amos Jeffries wrote:
Two patches:
Did not see the second patch attached. Was it send in a separate "pconn
changes" email?
patch #1:
This builds on the changes already audited in src/comm and src/base to
implement TCP listening ports being opened with ConnAcceptor and a
Subscription (to emit the HTTP or HTTPS client connect calls).
Alex; can you check that I have understood the IPC systems correctly?
+ /// handler to subscribe to Comm::ConnAcceptor when we get the response
+ Subscription::Pointer handlerSubscription;
It is not obvious what "response" we are talking about here. I would
remove the "when we ..." part or replace it with something like "after
we get the shared connection".
- int fd; ///< opened listening socket or -1
+ Comm::ConnectionPointer conn; ///< opened listening socket or -1
The comment became stale. "Or nil"?
It is also weird that we are using Connection to represent a listening
socket that is not really connected to anything. Are you sure this is
the right change for now? Or should this fd->conn change wait until we
have a proper Descriptor class perhaps?
+ if (sock_type == SOCK_STREAM) {
+ // TCP: setup the subscriptions such that new connections accepted by
listenConn are handled by HTTP
+ AsyncJob::Start(new Comm::ConnAcceptor(cbd->conn, FdNote(fdNote),
sub));
+ } else if (sock_type == SOCK_DGRAM) {
+ // UDP: setup the listener socket, but do not set a subscriber
+ Comm::ConnectionPointer udpConn = listenConn;
+ comm_open_listener(sock_type, proto, udpConn, FdNote(fdNote));
+ } else {
+ fatalf("Invalid Socket Type (%d)",sock_type);
+ }
This is out of place in Ipc::StartListening(), IMO. Ipc should not know
or care what kind of listener is being setup. Listener-specific
discrimination should be done at a higher level or, at the very least,
at Comm level. Ipc just shovels the descriptors to and from.
Please also note that the StartListening() description does not mention
that SOCK_DGRAM and SOCK_STREAM (and "other") are handled very differently.
+ if (sock_type == SOCK_STREAM) {
+ // TCP: setup the subscriptions such that new connections accepted by
listenConn are handled by HTTP
+ AsyncJob::Start(new Comm::ConnAcceptor(cbd->conn, FdNote(fdNote),
sub));
+ }
...
+ cbd->errNo = cbd->conn->isOpen() ? 0 : errno;
Will cbd->conn->isOpen() always be true for SOCK_STREAM here? If not,
then errno should not be used. If yes, perhaps we do not need to set
cbd->errNo in the SOCK_STREAM context?
=== modified file 'src/ipc/Strand.cc'
--- src/ipc/Strand.cc 2010-07-06 23:09:44 +0000
+++ src/ipc/Strand.cc 2010-09-24 09:29:47 +0000
@@ -6,13 +6,14 @@
*/
#include "config.h"
+#include "base/Subscription.h"
#include "base/TextException.h"
+#include "comm/Connection.h"
#include "ipc/Strand.h"
#include "ipc/Messages.h"
#include "ipc/SharedListen.h"
#include "ipc/Kids.h"
Are all of the above changes needed even though there are no other
changes in src/ipc/Strand.cc?
>
> -
> CBDATA_NAMESPACED_CLASS_INIT(Ipc, Strand);
>
-
-
Please avoid unnecessary whitespace changes in functionality patches.
They just increase the probability of a conflict with other pending patches.
- options(COMM_NONBLOCKING),
- fd_(-1)
+ options(COMM_NONBLOCKING)
Sometimes you replace FD initialization with connection(NULL)
initialization and sometimes you do not. Please be consistent.
- int fd(); ///< creates if needed and returns raw UDS socket descriptor
+ Comm::ConnectionPointer &conn(); ///< creates if needed and returns raw
UDS socket descriptor
...
- int fd_; ///< UDS descriptor
+ Comm::ConnectionPointer conn_; ///< UDS descriptor
Stale "descriptor is not a connection" comment and the
descriptor-vs-connection problem again.
Thank you,
Alex.
