Re: icap devel branch feedback

Hello Robert,
Sorry for this delayed reply.. took off - an early weekend:-)

Thanks for going through the icap branch.. One quick comment is that the
sources on that branch are not current. We keep the CVS current only at
http://sf.net/projects/icap-server. Sorry about that. I wanted to update
the sources here too - but did not find time for performing the HEAD
update and testing it out again... Will try to do so shortly.

Nevertheless some of your comments are still valid... please find my
comments inline..

>
> Geetha,
> Some feedback on the iCAP branch follows. If you have any
> questions/disagreements I'd love to hear them.
>
> You have multiple occurences of #undef HS_FEAT_ICAP in acconfig.h, and
> likewise in client_side.c.
>
> Changing the default access mode for icp and http to allow all is bad -
> it will not be acceptable for merging into HEAD like this.
 
OK - agreed! I will change that back to original one here. I made it
that way on the icap site, to make it easier for people to try out
things with the framework.

>
> You appear to only support one of reqmod and respmod at a time. This
> appears quite limiting - it should be possible to support both request
> and respond modification, indeed you should allow *multiple* occurences
> of both on a single request->response pair. I.e.: use foo reqmod for
> access control/redirection, then virus scan the response before caching,
> and finally perform add-stripping on html documents before delivery to
> the user.

The version on the icap-server sourceforge project supports both reqmod
and respmod. However, out the four possible vectoring points (pre cache
reqmod, post cache reqmod, precache respmod and postcache respmod), the
precache respmod is not supported, and pre cache reqmod is not yet fully
tested (available as a patch only). Multiple reqmod's and multiple
response modifications at a particular vectoring point can be supported
in the icap server side. The example you mentioned requires support for
content modification at all VPs - will be there in one of the future
releases ;-)
  
>
> You can avoid the global registration of the icap CBDATA by using the
> CBDATA_TYPE and CBDATA_INIT_TYPE or CBDATA_INIT_TYPE_FREECB macros
> within a single translation unit. This can be quite handy where only a
> single file needs to create cbdata objects of a given type.

Thanks for the suggestion - i'll look into this.

> Rather than using private functions from forward.c, http.c and
> client_side.c, consider generalising the relevant file to allow it's
> current external API to do what you need it to. You can also consider
> adding hooks for callbacks to allow you more control over it's
> behaviour. (Don't work on client_side - I've done a lot there that
> should hit HEAD soon :}).

Yes. I should do that sometime. Infact I did start that way - plugging
things in as callbacks - but faced a lot of problems. Let me check out
after your changes are into the HEAD.
However, I may still need to use some of the functions in http.c as the
encapsulated messages are all HTTP .. instead of rewriting the HTTP
header parsing routines, i thought i could as well leverage

Thanks for the useful feedback...
regards
Geetha

>
> Cheers,
> Rob
>
>
Received on Mon Aug 19 2002 - 00:51:28 MDT