Re: [PATCH] AsyncCall and CommCalls changes for cleanup-comm work.

On 09/23/2010 10:05 AM, Amos Jeffries wrote:
> Surprisingly small. These are the changes to src/base and CommCalls.*
> where all the Call related code has been changed.
>
> Including the generalized Subscription mechanism used by ConnAcceptor.
>
> CommCalls.* is still a work in progress. I'm confidant that much more of
> the code there can be removed as the cleanup progresses in the other
> components to make them use AsyncCalls more generically.
>
> The changes visible there are indicative of most of the changes to the
> wider squid code.
>
>
> Alex: removing the XXX marked const problems and testing they will work
> is blocked behind some build errors from the IOCB definition change. As
> soon as I have building code that wont hide const side-effects I'll be
> removing that muckup.

> +typedef void IOACB(int fd, const Comm::ConnectionPointer &details, comm_err_t flag, int xerrno, void *data);
> +typedef void CNCB(const Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data);
> +typedef void IOCB(const Comm::ConnectionPointer &conn, char *, size_t size, comm_err_t flag, int xerrno, void *data);

It would be nice if each of the three names is deciphered in lieu of
documentation for the types we are defining here.

> public:
> void *data; // cbdata-protected
> - int fd;
> + Comm::ConnectionPointer conn;
> + int fd; // raw FD from legacy calls. use conn instead.

Having both fd and conn is bad, but having them pretty much undocumented
makes it impossible for me to review the patch: I cannot review the
sometimes complex interactions between the two in the rest of the patch
because I do not understand when we should have none, one, the other,
and both members. Did I miss some documentation piece explaining this?

> @@ -65,12 +69,6 @@
> {
> public:
> CommAcceptCbParams(void *aData);
> -
> - void print(std::ostream &os) const;
> -
> -public:
> - ConnectionDetail details;
> - int nfd; // TODO: rename to fdNew or somesuch
> };

Why was nfd removed? Is it not specific to the accept callback?

> // connect parameters
> @@ -80,11 +78,6 @@
> CommConnectCbParams(void *aData);
>
> bool syncWithComm(); // see CommCommonCbParams::syncWithComm
> -
> - void print(std::ostream &os) const;
> -
> -public:
> - DnsLookupDetails dns;
> };

Where did the dns details go? Are they now supplied using some other API?

> // read/write (I/O) parameters
> @@ -176,10 +169,12 @@
> {
> public:
> typedef CommAcceptCbParams Params;
> + typedef RefCount<CommAcceptCbPtrFun> Pointer;
>
> CommAcceptCbPtrFun(IOACB *aHandler, const CommAcceptCbParams &aParams);
> + CommAcceptCbPtrFun(const CommAcceptCbPtrFun &o);
> +

.

> void dial();
> -

Please avoid whitespace changes in functionality-focused patches.

> + inline CommCbFunPtrCallT(const Pointer &p) :

The "inline" keyword is not needed if you are inlining directly after
the declaration.

> + AsyncCall(p->debugSection, p->debugLevel, p->name),
> + dialer(p->dialer)
> + {}
> +

.

> === modified file 'src/base/AsyncCall.cc'
> --- src/base/AsyncCall.cc 2009-04-09 22:31:13 +0000
> +++ src/base/AsyncCall.cc 2010-09-10 12:36:02 +0000
> @@ -77,4 +77,3 @@
> AsyncCallQueue::Instance().schedule(call);
> return true;
> }
> -

Please avoid whitespace changes in functionality-focused patches.

I will comment on the Subscription class separately.

Thank you,

Alex.
Received on Tue Oct 05 2010 - 03:50:54 MDT