Re: [PATCH] Cleanup comm connection setup layering

Amos Jeffries wrote:
> Alex Rousskov wrote:
>> On 06/09/2010 05:44 AM, Amos Jeffries wrote:
>>> (since last submission I've implemented most of Alex suggestions
<snip>
>
>>
>> * Do we really need to support Comm::Connection copying and assignment?
>> Seems like a very dangerous operation given the fact that Connection
>> closes it descriptor upon destruction. For example, the following code
>> is currently possible but is totally wrong:
>>
>> void foo(Connection c) { ... }
>>
>> Connection c;
>> c.fd = openex...
>> foo(c);
>>
>> If we do not use Comm::Connection copying and assignment now, I would
>> strongly recommend prohibiting it. Otherwise, please point me to the
>> code that does.
>
> Um. Ident needs to duplicate the received Comm::Connection before
> playing with some of its fields and using the raw received data.
> I'll make that a memcpy() and unset the required fields instead of
> assignment copy.

meh. we can't memcpy externally due to the cbdataReference on _peer.
Added a specific clone() call instead.

<snip>
>>
>> * ConnectStateData is not owned by anyone, deletes self, and is
>> scheduling/receiving async calls. It should be converted into an
>> AsyncJob with proper start/done methods. As a free bonus, this will
>> greatly help with debugging.
>
> Sweet. Underway.
>
> When I get my head around the Dialer parameters usage, those
> ConnectCbFunPtr will die as well.
>

After another short look at it, since connect and Accept are both
producing a Comm::Connection + the common details I think the
CommConnectParams and CommAcceptParams IO parameters can now be deflated
down to CommCommonParams.

Is there any specific Async / Dialer reason why they are separate?

<snip>
>>
>>
>> * Note how the dual-use Connection prompts you to allocate and then get
>> rid of a "temporary" connection in the ICAP code below:
>>
>>> + connection = io.conn;
>>
>> This waste would not exist if we did not use Connection objects as
>> destination detail holders.

This is a refcount ptr copy.

NP: io.conn is a Comm::ConnectionPointer to the same object created in
Adaptation::Icap::Xaction::openConnection() with "connection = new
Comm::Connection" just before calling ConnectStateData->connect().

You asked me to set "connection" field there instead of allocating a
temporary local and passing to the ConnectStateData (to be returned
after opening via io.conn and saved in connection at that point).
  NP: for now removing the original io.conn saving to connection.

Between ConnectStateData creation and the io.conn line above the ICAP
"connection" object is in the opening but still maybe-closed or
maybe-half-open state you seem so worried about in the later code.

>>
>> * Adaptation::Icap::Xaction::fillPendingStatus and
>> Adaptation::Icap::Xaction::fillDoneStatus will now crash when connection
>> pointer is NULL. Please fix.
>>

Done.

>>
>> * Please move (connection != NULL && connection->isOpen()) into a
>> dedicated haveConnection() const method instead of duplicating the same
>> code many times.
>>
Done.

>>
>> * Xaction is not the only ICAP code using the connection member. Please
>> fix all the affected code. For example:
>>
>>> void Adaptation::Icap::ModXact::startReading()
>>> {
>>> Must(connection >= 0);
>>

Done as far as I can tell.

>>
>>
>> * Please shorten the comment below to "///< ICAP server connection" even
>> though it is currently not very accurate if we are not reusing a pconn
>> and have not connected yet (due to dual-use of Connection).
>>
>>> - int connection; // FD of the ICAP server connection
>>> + Comm::ConnectionPointer connection; // Handle to the ICAP
>>> server connection
>>

Done.
NP: re-using a pconn then we are connected as soon as the pconn is found
and FD set.
The other case is only inaccurate due to setting "connection" field
before ConnectStateData has returned as result. :(

>>
>> * Does connect_retries apply to ICAP connections? If yes, the
>> "forwarding" description below is a little misleading.
>>
>>> + This sets the maximum number of connection attempts for each
>>> + potential host address selected by forwarding.
>>
>> Can we clarify how one can get multiple potential host addresses here?
>> And whether we are talking about multiple domain names or multiple IP
>> addresses?
>>

Done.

>>
>> * Not a big deal, but I would rather see us _removing_ such personal
>> artifacts than adding more of them:
>>
>>> + * AUTHOR: Amos Jeffries
>>> + * Copyright (c) 2010, Amos Jeffries <amosjeffries_at_squid-cache.org>
>>
>> Technically, we can all add our names to the majority of Squid files
>> because any active developer added at least something there, but it only
>> wastes ink and time, IMO.

Well, IMHO we need to complete last years discussions about forming an
officially registered developer group that can own the copyright. Then
that one name can go in all our joint files.

Until then, I'm adding explicit mention of all people I know have made
significant contributions to each individual file.

>>
>> * Open paren in the comment below. Why not use C++ // one-line comments
>> for one line comments?
>>
>>> /* set the new one (unless it is NULL */
>>

meh. Spent too much time editing the old C code in helpers recently.

>>
>> * Please remove Comm::PathsPointer typedef. It is very misleading
>> because we name refcounting pointers that way. Most of my review, I
>> assumed it is a refcounting pointer... I probably missed bugs because of
>> that.

Done. Sorry it was cruft from the initial failed attempt to make paths
refcounted too.

>>
>>
>> I have not fully reviewed all the code but I hope the above is
>> sufficient to help you move forward. Please CC me if you post a new
>> version for review.

It is. Will do. Thank you.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.4