Re: [PATCH] Cleanup comm connection setup layering

On 06/25/2010 10:09 PM, Amos Jeffries wrote:
>>> 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.

Just do not call it clone() if it does not copy _everything_. Call it
cloneMeta() or something like that.

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

In the code I have reviewed, the connect and accept parameters as well
as the corresponding methods are different. If they are no longer
different, there is probably no reason to have two different classes.
Mind the syncWithComm() method though.

>>> * 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.

This is an assignment. If "connection" is already the same as "io.conn",
then the assignment does nothing (if you do not count reader's
confusion). If they are different, then the assignment de-allocates the
temporary connection object. If the updated code has neither of these
problems, that's good, of course.

>>> * 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.

I trust you compile with ICAP support enabled. I hope "connection >= 0"
leads to compilation errors. If not, we may need to fix our refcounting
pointer class.

HTH,

Alex.
Received on Sat Jun 26 2010 - 20:08:41 MDT