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

 On Tue, 08 Mar 2011 17:00:54 -0700, Alex Rousskov wrote:
> 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.

 I disagree with that view since a rename touches as much and the same
 code as a member change does.
 But don't let that hold up commit.

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

 Ah, -lecap a flag?
  That is a MUST for adding to LDADD instead of FLAGS according to the
 autotools docs. I found that while investigating kerberos helper build
 failures on 3.1. Don't want to add those same failures again for ecap.

 -L is apparently optionally usable in both FLAGS and LDADD. We in Squid
 have a tendency to place it in LDADD immediately before the library
 which uses it. But we take what pkg-config gives us on that.

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

 Looks like a dirty Makefile or build area to start with.
 ar is trying to play with adaptation/ecap/libecap.la instead of the
 renamed new adaptation/ecap/libxecap.la

 I will take a closer look later this evening and see if I can find
 anything.

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

 Well, I expect (c) too, that is what my experience with it elsewhere
 has been despite libtool.

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

 The stricter toolchains skip/discard or warn for any -l entries before
 "-o a.out". The result is them failing to find and link symbols from the
 external library.
 -L seems to be irrelevant but useful to pair with its -l wherever that
 goes.

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

 Okay, if we are going to make that distinction I think it needs to be
 done uniformly and documented. (out of scope)

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

 Understood. Some local documentation comment pointing at where to find
 that ecap documentation would do then.

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

 If you can for now, just to be consistent and clear in the startup.
 Long term I think we need to decide on some other way/place to output
 these startup/reconfigure/shutdown notices.

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

 Very much in scope IMO.
 Alter the option name means altering all its public references to
 match, even if they started off as typo versions of it.

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

 That works. Fixed.

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

 Have not looked at the new code yet. Will do in the next few hours and
 get back to you ASAP.

 Amos
Received on Wed Mar 09 2011 - 01:47:10 MST