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 <[email protected]>
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
