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

On 05/10/10 16:50, Alex Rousskov wrote:
> 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.
>

You wrote what documentation there is in the block above them. Semantics
have not changed. FD is now a member of conn, that is all.

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

All the new code (ConnAcceptor, ConnOpener, IPC, write calls, read
callbacks) uses conn. The code I have not got to transitioning yet (read
callers/timeout/close calls) sets and reads 'fd'.

  The 'complex interactions' only exists on dialing the IOCB. Where it
makes sure that if one is set by the scheduling code the other is a mirror.

The special case (for now) is IOACB dialer where fd contains the (mostly
unused listening socket FD) and conn contains the newely accepted
connection descriptor.

see below.

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

Because having two FD and a conn was worse than one fd and a conn.

Also most of the local-IP/remote-IP/flags fetching details are now
hidden inside ConnAcceptor::connected() instead of duplicated in the
callback handlers. When the shuffling is complete 'fd' field will die
too and the callers will use their stored listenConn (instead of fd) and
the parameter io.conn where necessary instead of fd and nfd respectively.

> Is it not specific to the accept callback?

It is/was. Fully superseded by the conn member. Which is currently still
called "details" parameter to IOACB.

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

They jumped out of Comm layer, up past ServerStateData and FwdState then
sideways into peer_select.cc. Connection opening is post route
selection and thus far too late for DNS resolving.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2