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

On 03/04/2011 10:57 PM, Amos Jeffries wrote:
> 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?

Comment rewritten as:

> dnl Use EXT prefix so that make and libtool messages distinguish between
> dnl external libecap (that we check for here) and our own convenience lib.

When make or libtool fails, having distinct names for make variables and
libraries helps with deciphering raw Makefiles, build log, and libtool
error messages.

For example, when libtool says "libecap.lax not found", and we have
external libecap and internal libecap, we cannot tell whether that
libtool-generated .lax file is specific to the external libecap or our
own libecap. This particular (and real) example illustrates why our
convenience library was renamed to libxecap, but EXT prefix serves the
same purpose when working with Make files/logs.

At some point of my libtool struggles, I thought that libtool and/or
make _need_ a custom EXT prefix, but I believe adding it did not solve
the core problem (which is also discussed further below).

I can remove EXT prefix if it bothers anybody. I believe everything will
still work without it. It is just a debugging aid.

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

Not changed: I agree that polishing AccessLogEntry would be nice, but
this particular patch does not add new members to AccessLogEntry. It
only renames an existing member. I can volunteer to polish later, but I
believe such polishing is outside this project scope.

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

Removed.

IIRC, HERE does not contain the function name on some platforms though.

> src/adaptation/Config.h:
> * on createService() you are not using const reference for the
> parameter.

Fixed.

> Same for ParseServiceGroup(). Any particular reason?

No good reason I can think of. This patch does not change
ParseServiceGroup(). It is possible that the member profile was copied
from some old callback profile that needed a non-reference type. We may
support by-reference callback parameters now, and the call itself is not
a callback so this can be polished. Out of this project scope though.
Will try to polish later.

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

Changed based on your comments below, but I am not sure I got all of
them correctly. Since most "natural" macro combination (IMHO) does not
work with libtool, I cannot improve it further on my own. If something
else needs to be done, please let me know.

> * Why is an automake macro called *_LIBS being added to LDFLAGS?

Because the macro (EXTLIBECAP_LIBS) contains ld flags (-L and -l). The
macro is created by pkg-config, and Squid has no control over its name.
Furthermore, I suspect the _LIBS suffix is standard in this pkg-config
context.

I started with _LIBADD, but libtool fails when I add EXTLIBECAP_LIBS to
libxecap_la_LIBADD:

libtool: link: rm -fr .libs/libxecap.a .libs/libxecap.la
libtool: link: (cd .libs/libxecap.lax/libecap.a && /usr/bin/ar x
"/home/rousskov/Edit/squid3/3p2-ecap/src/adaptation/ecap/.libs/libecap.a")
/usr/bin/ar:
/home/rousskov/Edit/squid3/3p2-ecap/src/adaptation/ecap/.libs/libecap.a:
No such file or directory
make: *** [libxecap.la] Error 9

The above is for adding EXTLIBECAP_LIBS to libxecap_la_LIBADD in
adaptation/ecap/Makefile.am, not in adaptation/Makefile.am. However,
adding EXTLIBECAP_LIBS to adaptation_la_LIBADD (in
adaptation/Makefile.am) seems to work. So that is what I did to address
your comment. Please let me know if that was wrong.

If you can make it work using a more natural arrangement, please let me
know how.

> I can't see where it is being set to know if its wrongly named or
> wrongly copied.

pkg-config sets EXTLIBECAP_LIBS.
pkg-config does not set a corresponding _FLAGS variable.

We can remove pkg-config support if you do not like the side-effects.
FWIW, I added it because:

  (a) We need libecap version checks.
  (b) I thought you wanted it because you requested it on eCAP.
  (c) I expected it to simplify things.

My expectations in (c) were totally ruined by libtool, but it is
possible that I am just using pkg-config and/or libtool wrong.

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

Since EXTLIBECAP_LIBS just contains -L and -llibecap options, pointing
to the _external_ libecap library, I do not think it should
affect/create dependencies. Am I wrong? Or misinterpreting what you want
me to change?

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

Changed.

These are C preprocessor flags, not C++ compiler flags though.
pkg-config does not set compiler flags so this change feels wrong to me.

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

All fixed?

> src/adaptation/ServiceConfig.cc:
> * in Adaptation::ServiceConfig::grokExtension() please use DBG_CRITICAL
> for 0-level debugs instead of "0".

Fixed.

> - it also needs a suitable WARNING/ERROR/FATAL etc. to highlight the
> effect of leaving it.

Fixed. I used ERROR because it is an error. Whether it is FATAL, the
calling code should decide (and probably does -- no change here).

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

Agreed, but out of scope. This will be polished separately.

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

It is libecap responsibility to document this and all other
libecap::Header APIs. We are just implementing them. Will add the
corresponding comment for all MessageRep virtual members as a group, but
that change is outside this project scope.

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

Sure, but this patch does not touch the "starting eCAP service" line.
Will change separately later.

> If its just a startup/restart then
> it might even go at level-1 with the other module starting/stopping
> messages.

Yes, this code is about adaptation service activation (once per Squid
[re]configure). Do you want me to use DBG_IMPORTANT there?

> 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"

Out of scope. Will fix later.

> * in the documentation for adaptation_masterx_shared_names please split
> the ICAP and eCAP methods of setting into two paragraphs.

Changed.

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

Fixed?

USE_ADAPTATION is enabled by --enable-icap-client and/or --enable-ecap
so I used:

define["FOLLOW_X_FORWARDED_FOR&&USE_ADAPTATION"]="--enable-follow-x-forwarded-for
and (--enable-icap-client and/or --enable-ecap)"

Please let me know if a different format is preferred.

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

Will do, but changing how we log adaptation headers is out of this
project scope. Here, we just move the existing and working ICAP code
into the more general adaptation area so that eCAP can use it. Those
XXXs are bugs discovered while working on this patch but are not this
patch bugs. IMO, fixing these bugs would affect ICAP users (that may not
care about eCAP) and should be done separately, with a dedicated change
log entry, etc.

I have attached the adjusted cumulative merge bundle and an incremental
patch that highlights just the above fixes. If you need more changes or
disagree with my scope decisions, please let me know. Otherwise, I will
commit this and create to-dos for the problems declared out of scope.

Thank you,

Alex.