Re: [AUDIT PATCH] comm cleanup - src/ipc/ changes

On Mon, 04 Oct 2010 12:46:07 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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?

Yes sorry. I found a bug before attaching and forgot to update the text.

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

Okay. Fixing.

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

The listening socket has all the same fields and connection management
requirements as any other read-only socket, but remote IP being nil.

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

I was a little worried about this. The problem is that we don't yet have a
UDP acceptor/listener/reader object. and I don't want to bloat this commit
any more than I have to. ConnAcceptor is for TCP accept() handshakes only
so to spawn them from IPC this if() is required on one form or another.

So the question is whether:

 #1) IPC should be spawning ConnAcceptor and the UDP class (in which case
the if stays) since they are not needed until the FD has arrived, or...

 #2) if their first action should be for the caller to create the relevant
ConnAcceptor/UDP themselves, subscribe. With ConnAcceptor/UDS running IPC
in their start() to fetch the FD

I picked (1) for the submission patch since you had IPC calling
comm_open_listener() in trunk.

>
> Please also note that the StartListening() description does not mention
> that SOCK_DGRAM and SOCK_STREAM (and "other") are handled very
differently.
>

They are not very different. Only the "other" case with its fatal().

AsyncJob::Start(new Comm::ConnAcceptor(...)) is the wrapper of
comm_open_listener(SOCK_STREAM, ...). IPC is then finished and returns the
listening-done call. The big difference is in the followup async steps
outside of IPC.

Outside of IPC;
 the UDP listen-done handles the packet.
 the TCP listen-done waits for any subscription calls to arrive.

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

Not.
If ConnAcceptor::start(){comm_open_listener(){comm_openex()}} fails for
any reason it will be false.

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

Yes. gcc informs that Strand.cc is instantiating a template (SharedListen
IIRC) which now has both of the above as dependencies.

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

doh. Thanks.

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

Will fix.

Thank you.

Amos
Received on Mon Oct 04 2010 - 23:38:43 MDT