Re: [PATCH] Support libecap v0.2.0; fixed eCAP body handling and logging

On 21/02/11 18:38, Alex Rousskov wrote:
> Hello,
>
> The eCAP project recently released two new versions of the eCAP
> library, with several important features added. The attached patch adds
> Squid support for the latest libecap v0.2.0 and fixes a few bugs we
> found along the way. I will summarize the changes below. My
> lp:~rousskov/squid/3p2-ecap branch contains detailed messages for these
> changes.
>
> The new eCAP features revolve around adapter configuration (similar to
> ICAP OPTIONS exchange) and transaction meta-information (similar to ICAP
> message headers, not to be confused with ICAP-encapsulated HTTP
> headers). There is also request satisfaction and message blocking
> support. These features are needed for many adaptation projects. For
> example, with these additions, we were able to write a ClamAV virus
> scanning adapter (to be published after this work is completed).
>
>
> Summary of changes:
>
> libecap v0.2.0 support: accept/update/log eCAP transaction meta-info.
> libecap v0.2.0 support: supply client IP and username to eCAP adapter.
> libecap v0.1.0 support: Support blockVirgin() API with ERR_ACCESS_DENIED
>
> Use pkg-config's PKG_CHECK_MODULES to check for and link with libecap.
>
> Support adapter-specific parameters as a part of ecap_service
> configuration. Allow uri=value parameter when specifying adaptation
> service URIs.
>
> Fixed virgin body handling in our eCAP transaction wrapper
> (Ecap::XactionRep). Fixed BodyPipe.cc:144 "!theConsumer" assertion.
>
> Log "important" messages from eCAP adapters with DBG_IMPORTANT not DBG_DATA!
>
> Added XXXs to identify old unrelated problems to be fixed separately.
>
>
> Thank you,
>
> Alex.

Okay the polish audit results...

configure.ac:
  * can you explain a bit more about what the pkg-check problem is with
library names and libtool?

src/AccessLogEntry.h:
  * please note the TODO on the "headers" member. New uses of that area
is not desirable. The ideal structure for AccessLogEntry has separate
sub-classes to match http::, icap::, adapt::, ecap:: namespaces in the
logformat tokens.

   Please add an "adapt" sub-class to match the namespace for
adapt::<last_h.

src/Server.cc:
  * HERE << "handleAdaptationBlocked: " is redundant. HERE contains the
function name.

src/adaptation/Config.h:
  * on createService() you are not using const reference for the
parameter. Same for ParseServiceGroup(). Any particular reason?

src/adaptation/Initiator.cc:
  * please enact that TODO: Move to src/adaptation/Answer.*

src/adaptation/Makefile.am:
  * Why is an automake macro called *_LIBS being added to LDFLAGS?
   I can't see where it is being set to know if its wrongly named or
wrongly copied.
   - the comment in src/adaptation/ecap/Makefile.am indicates its
wrongly used. Since adding *_LIBS and *_LDADD auto-magically updates
dependencies. Adding *FLAGS does not.

src/adaptation/ecap/Makefile.am:
  * please use CXXFLAGS instead of CPPFLAGS like we do everywhere else

src/adaptation/Service.h,cc:
  * please use const ServiceConfigPointer& for passing to constructor.
(less refcounting overheads as you keep pointing out to me).
  - same in other places too.

src/adaptation/ServiceConfig.cc:
  * in Adaptation::ServiceConfig::grokExtension() please use
DBG_CRITICAL for 0-level debugs instead of "0".
  - it also needs a suitable WARNING/ERROR/FATAL etc. to highlight the
effect of leaving it.

src/adaptation/ecap/MessageRep.cc:
  * the includes seem to be violating our policy on "" before <>. Not a
major for this patch, but it may be hiding bugs and needs fixing. Same
for a lot of these adaptation files.

src/adaptation/ecap/MessageRep.h:
  * zero documentation on visitEach() - what does it actually do?
    pass the headers freshly received from ecap back to ecap? part of
the chaining? something else?

src/adaptation/ecap/ServiceRep.cc:
  * I think the starting eCAP service: message should be at level 2. Out
of regular sight but easy to get to. If its just a startup/restart then
it might even go at level-1 with the other module starting/stopping
messages.

src/cf.data.pre:
  * while you are updating the documentation for
icap_client_username_header please correct the typo which mentions a
non-existent "send_username" to be "adaptation_send_username"
  * in the documentation for adaptation_masterx_shared_names please
split the ICAP and eCAP methods of setting into two paragraphs.

src/cf_gen_defines:
  * missing entry for FOLLOW_X_FORWARDED_FOR&&USE_ADAPTATION to generate
readable documentation on adaptation_uses_indirect_client

src/log/FormatSquidCustom.cc:
  * in LFT_ADAPTATION_LAST_HEADER you haev an XXX. Please resolve it for
the new cases. No need to perpetuate the error.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.11
   Beta testers wanted for 3.2.0.5