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

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.
Received on Mon Oct 04 2010 - 18:46:19 MDT