Re: [PATCH] comm cleanup mk8 - src/comm only

On 09/22/2010 08:28 AM, Amos Jeffries wrote:
> On 21/09/10 10:00, Alex Rousskov wrote:
>> On 09/19/2010 05:35 AM, Amos Jeffries wrote:

>> * I do not like how Comm::ConnOpener::connect() calls itself. I assume
>> you did not invent this, but the code will fail to produce the
>> intended(?) delay if there are no other async calls already scheduled,
>> which is something the class does not really control. Recall that async
>> calls are AsyncCallQueue::fire()d until there are none left...
>>
>> At the very least, please add a comment explaining why we use such a
>> hack instead of either using an explicit loop or waiting a little bit
>> via eventAdd.
>>
>
> I think you asked me to remove eventAdd a while back before this was a
> Job. Reverted to the old .05 sec delay.

I could have made the wrong call, you could have interpreted my comment
incorrectly, or both. If you want to review what happened, please find
and repost that conversation.

>> * I may be wrong, but the interesting "Is opened conn fully open?"
>> comment below may reveal a couple of bugs:
>>
>>> + // recover what we can from the job
>>> + if (conn_ != NULL && conn_->isOpen()) {
>>> + // it never reached fully open, so abort the FD
>>> + conn_->close();
>>> + ...
>>
>> Clearly, conn_ is open inside that if-statement (there is no such thing
>> as half-open conn, I hope; we have enough headaches from half-closed
>
> sorry, no such luck. see COMM_INPROGRESS.

I see. Fortunately, my swanSong() fix still applied.

> I also see that I missed clearing the write handler before fully closing
> conn_. do the timeout and closing handlers need to be explicitly
> removed? or are they happy with the cancels?

AFAIK, comm_close() will remove those handlers automagically because it
will call them with COMM_ERR_CLOSING. However, our code needs to make
sure that it can handle such callbacks until swanSong() is called.

> Considering no other jobs etc have anything validly pending on this
> brand new FD should I be calling fd_close(conn_->fd) here instead of
> conn_->close()?

In general, if you used comm_open, use comm_close.

If comm_close works, let's use it, avoiding low-level calls we should
not even have access to. Comm itself may have something pending on that
new FD...

>> * Can you explain how "syncWithComm() side effects hit
>> theCallSub->callback()"? Is not theCallSub->callback() itself constant
>> and its result is stored in a local variable? I do not understand how
>> syncWithComm() can affect theCallSub itself. Thank you.
>>
>
> Possibly obsolete now.
>
> Dialer copy-constructors was initially created using getDialer() which
> call syncWithComm() which in CommIoCb child class updates this->* state.
> Fallout of that is that:
> the CommIoCb constructor cant take a const parameter,
> the parent class cant provide a const overload constructor,
> the other children cant either,
> callback() using copy-constructor cant be const.
>
> I *think* the copy constructors are okay using raw dialer pointers now.
> Which means everything down the chain can be const.

I cannot comment on that without seeing the new call-copying code.

>> * cbdataReferenceDone() already sets its parameter to NULL. No need to
>> do that twice:
>>> + /* clear any previous ptr */
>>> + if (_peer) {
>>> + cbdataReferenceDone(_peer);
>>> + _peer = NULL;
>>> + }
>>
>
> Great.
> At this random point we are not sure its valid. For now I've make the
> if() use the fixed getPeer().

That would leak _peer if it became invalid (unless you call
cbdataReferenceDone(_peer) in getPeer() now, which would be technically
correct and efficient but weird).

> Would it be correct to use
> cbdataReferenceValidDone(NULL, _peer) unconditionally here with no
> callback? instead of separate validate and done?

You do not care about _peer validity or value because you are not
dereferencing that pointer. Just do:

     cbdataReferenceDone(_peer);

No ifs, no setting to NULL, no getPeer().

>> * Consider s/_peer/peer_/ in Connection for consistency with most other
>> code.
>
> The "struct peer_" global symbol clashes and gcc complains about missing
> ; or = after variable "struct".
> I don't plan on fighting that one just yet. Will mark it to be done though.

Agreed. The struct should be renamed (to Peer?), but that is not your
problem. You can use just "peer" for the member name, I guess.

>> * s/unused//g. C++ does not require naming unused parameters.
>
> Done. And converted the two places (commSetSelect and eventAdd wrappers)
> to use JobCallback with NullaryMemFunT dialers on connect().

Not sure what you mean. Will become clear when the code is posted, I guess.

Cheers,

Alex.
Received on Wed Sep 22 2010 - 16:49:07 MDT