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
