ACLFilledChecklist::checkCallback() not needed?

Hello,

    Does anybody want to keep ACLFilledChecklist::checkCallback()? Long
time ago, that method was used to unlock auth_user_request(s) via
AUTHUSERREQUESTUNLOCK() to avoid memory leaks. Today, we use refcounting
pointers for auth_user_requests so they should not leak even without an
explicit NULL assignment.

The method, specifically its questionable conn()->auth_user_request
part, may be in the way of authentication because it results in
forgetting connection's authentication state under some conditions. I
would like to remove it. Any objections?

The ACLChecklist::checkCallback() will stay, of course, but it will no
longer be virtual.

I quoted current and the "original" code below for your convenience.

Thank you,

Alex.

current code (that I would like to remove):

> void
> ACLFilledChecklist::checkCallback(allow_t answer)
> {
> debugs(28, 5, HERE << this << " answer=" << answer);
>
> #if USE_AUTH
> /* During reconfigure, we can end up not finishing call
> * sequences into the auth code */
>
> if (auth_user_request != NULL) {
> /* the filled_checklist lock */
> auth_user_request = NULL;
> // It might have been connection based
> // In the case of sslBump we need to preserve authentication info
> // XXX: need to re-evaluate this. ACL tests should not be playing with
> // XXX: wider scoped TCP connection state, even if the helper lookup i!
> if (conn() && !conn()->switchedToHttps()) {
> conn()->auth_user_request = NULL;
> }
> }
> #endif
>
> ACLChecklist::checkCallback(answer); // may delete us
> }

which originated from the middle/unlocking part of this code (see trunk
r9509.1.29):

> void
> ACLChecklist::checkCallback(allow_t answer)
> {
> PF *callback_;
> void *cbdata_;
> debugs(28, 3, "ACLChecklist::checkCallback: " << this << " answer=" << answe
>
> /* During reconfigure, we can end up not finishing call
> * sequences into the auth code */
>
> if (auth_user_request) {
> /* the checklist lock */
> AUTHUSERREQUESTUNLOCK(auth_user_request, "ACLChecklist");
> /* it might have been connection based */
> assert(conn() != NULL);
> /*
> * DPW 2007-05-08
> * yuck, this make me uncomfortable. why do this here?
> * ConnStateData will do its own unlocking.
> */
> AUTHUSERREQUESTUNLOCK(conn()->auth_user_request, "conn via ACLChecklist"
> conn()->auth_type = AUTH_BROKEN;
> }
>
> callback_ = callback;
> callback = NULL;
>
> if (cbdataReferenceValidDone(callback_data, &cbdata_))
> callback_(answer, cbdata_);
>
> delete this;
> }
Received on Wed Jun 27 2012 - 04:17:30 MDT