Re: squid-3 icap related patch

On Thu, 2006-09-28 at 05:18 -0400, Tsantilas Christos wrote:

> 3) File ICAP/ICAPModXact.cc function
> ICAPModXact::handleCommWroteHeaders()
>
> Sometimes, we have read the headers but not any part of the body yet
> so in this case we must not call writeMore() function.
>
> - writeMore();
> + if(virgin->data->body && virgin->data->body->contentSize())
> + writeMore();

As far as I can see, virgin->data->body is never NULL at this stage and
the rest of the code assumes it is not NULL. Thus, I assume the first
part of the above if-statement can be safely removed. This leaves us
with the zero-size content part.

I believe zero size content is valid. If writeMore() cannot handle it,
writeMore() should be fixed and still called unconditionally in the
above code. One place where zero-sized content is useful is for
zero-size previews, which are common (please see below on that).

> 7) File ICAP/ICAPModXact.cc function ICAPModXact::estimateVirginBody
>
> if function header->expectingBody(method, size) returns size==0 then
> we do not expect a body.

Can you explain why zero-size body should be treated differently here? I
am especially worried about the case were Squid may tell the ICAP server
that there was no HTTP entity body where in fact there was a body of
zero length. The difference is likely to affect the adaptation logic on
the ICAP server if the server adapts (or preserves) HTTP bodies.

Again, if zero-size bodies cause problems elsewhere in the ICAP code, we
should fix those problems, but hopefully without completely ignoring
such bodies.

> 8) File ICAP/ICAPModXact.cc function ICAPPreview::enable
>
> The theAd is the expected preview size so if theAd==0 do not
> enable preview mode.
>
> - theState = stWriting;
> + if(theAd >0)
> + theState = stWriting;
> + else
> + theState = stDisabled;

Zero-size Preview is very useful because it allows an ICAP server to
receive the HTTP headers without the body before making any decisions.
Why do you want to disable it? Do we have zero-preview handling bugs
elsewhere?

I believe I have applied all other changes you wrote about. The changes
currently live on the squid3-icap branch in the SourceForge Squid
repository. There are other ICAP bugs that must be fixed before ICAP
code can be declared stable. I hope to nail most in the next week or
so.

I would appreciate if you can answer the above questions and/or provide
your test cases so that we can "close" the three pending changes one way
or the other.

Thank you,

Alex.
Received on Mon Oct 02 2006 - 23:24:37 MDT