On Thu, 2012-02-02 at 09:49 -0800, Fox, Kevin M wrote:
> With a few tweeks here and there, I was just able to get it to build and
> properly parse a simple esi file.
>
> I'll post a patch in a few.
Here's the patch I promised. Its rough right now but should give you an
idea how to fix things.
It has the following things in it:
* Makefile fix to add missing files.
* Change return code checking to match whats trunk trafficserver.
* Include missing header files.
* Fix c++ namespace issues.
* Work around strange name mangling/linking issue.
* Force the assumption that the cached data is RAW_ESI, not PACKED_ESI.
Things wouldn't work without it.
* Comment out a block of code that looked to be incorrectly handling
EOF.
After this, simply loading the plugin and setting response header X-ESI
in apache httpd seems to work.
A few further bugs I have bumped into that aren't addressed in this
patch:
* It doesn't seem to parse gzip like it looks like it should. To work
around, I had to disable it in apache httpd with "RewriteRule . -
[E=no-gzip:1]"
* If the client requests gzip, the ESI processor will gzip the result.
It works in firefox but is invalid in chrome. Pulling a dump with curl
and running it through gzip --list shows it has the correct uncompressed
size and compressed size. using zcat shows the correct data but has the
warning: "invalid compressed data--length error". As far as I read the
gzip spec though, the raw binary file looks valid to me. Not sure what
this is. This can probably be simply disabled for now though.
* esi:include is slightly broken. You get all the data back properly but
sometimes the headers are sent prematurely with a Content-Length of
2**31-1. This causes clients to timeout and fail. I'm currently unsure
how to fix this.
I've tried a few of the more advanced esi features, including ensuring
cookies make it back to the origin server and things seem to work good.
So, once the above bugs are figured out (particularly the include one),
I think it will be in pretty good shape.
Thanks,
Kevin
> On Thu, 2012-02-02 at 09:43 -0800, Igor Galić wrote:
> >
> > ----- Original Message -----
> > > On 2/2/12 10:00 AM, Kevin Fox wrote:
> > > > Re-asking on the devel list instead.
> > > >
> > > > What is the current state of the trafficserver-plugins/esi plugin?
> > > >
> > >
> > > It needs to be updated / ported to the current code base / API at a
> > > minimum.
> > > I have no idea of the overall quality though.
> >
> > I'm pretty and sure that someone already attempted to port it to
> > 3.0.x/trunk -- IIRC, the reason why it's currently failing more than
> > terribly is.. oh dear
> >
> > lib/Stats.cc:45: if (!g_system->create($i)) {
> >
> >
> > Anyway, what we need to do is find a sensible solution for the lib
> > directory which ESI shares with a different plugin..
> > We should pull it out from there and integrate it in a top-level
> > Makefile. -- Maybe other plugins can profit from that lib stuff too..
> >
> > > -- Leif
> >
> >
> > So long,
> >
> > i
> >
>
>
diff --git a/esi/Makefile b/esi/Makefile
index cb083c2..5094429 100644
--- a/esi/Makefile
+++ b/esi/Makefile
@@ -17,7 +17,7 @@
TSXS?=tsxs
all: plugin.cc serverIntercept.cc
- $(TSXS) -v -Ilib -c $? -o esi.so
+ $(TSXS) -v -I lib -I fetcher/ -c plugin.cc serverIntercept.cc lib/*.cc fetcher/*.cc -o esi.so
install:
$(TSXS) -i -o esi.so
diff --git a/esi/fetcher/HttpDataFetcherImpl.cc b/esi/fetcher/HttpDataFetcherImpl.cc
index d352c65..1d53dab 100644
--- a/esi/fetcher/HttpDataFetcherImpl.cc
+++ b/esi/fetcher/HttpDataFetcherImpl.cc
@@ -103,11 +103,15 @@ HttpDataFetcherImpl::addFetchRequest(const string &url, FetchedDataProcessor *ca
event_ids.timeout_event_id = _curr_event_id_base + 2;
_curr_event_id_base += 3;
- if (TSFetchUrl(http_req.data(), http_req.size(), _client_addr, _contp, AFTER_BODY,
+//FIXME. This looks to be a regression.
+TSFetchUrl(http_req.data(), http_req.size(), _client_addr, _contp, AFTER_BODY,
+ event_ids);
+/* if (TSFetchUrl(http_req.data(), http_req.size(), _client_addr, _contp, AFTER_BODY,
event_ids) == TS_ERROR) {
TSError("Failed to add fetch request for URL [%.*s]", url.size(), url.data());
return false;
}
+*/
TSDebug(_debug_tag.c_str(), "[%s] Successfully added fetch request for URL [%.*s]",
__FUNCTION__, url.size(), url.data());
diff --git a/esi/lib/FailureInfo.cc b/esi/lib/FailureInfo.cc
index b3ee23e..35f1533 100644
--- a/esi/lib/FailureInfo.cc
+++ b/esi/lib/FailureInfo.cc
@@ -22,7 +22,7 @@
*/
#include "FailureInfo.h"
-
+#include <cstdlib>
static int LOWER_CUT_OFF=300;
static int HIGHER_CUT_OFF=1000;
diff --git a/esi/lib/Stats.cc b/esi/lib/Stats.cc
index 92d6910..c670ae0 100644
--- a/esi/lib/Stats.cc
+++ b/esi/lib/Stats.cc
@@ -25,7 +25,9 @@
using namespace EsiLib;
-const char *Stats::STAT_NAMES[Stats::MAX_STAT_ENUM] = {
+namespace EsiLib {
+namespace Stats {
+const char *STAT_NAMES[Stats::MAX_STAT_ENUM] = {
"esi.n_os_docs",
"esi.n_cache_docs",
"esi.n_parse_errs",
@@ -35,25 +37,33 @@ const char *Stats::STAT_NAMES[Stats::MAX_STAT_ENUM] = {
"esi.n_spcl_include_errs"
};
-static int g_stat_indices[Stats::MAX_STAT_ENUM];
-static StatSystem *g_system = 0;
+int g_stat_indices[Stats::MAX_STAT_ENUM];
+StatSystem *g_system = 0;
-void Stats::init(StatSystem *system) {
+void init(StatSystem *system) {
g_system = system;
if (g_system) {
for (int i = 0; i < Stats::MAX_STAT_ENUM; ++i) {
- if (!g_system->create($i)) {
+//FIXME doesn't return avalue.
+g_system->create(i);
+/* if (!g_system->create(i)) {
Utils::ERROR_LOG("[%s] Unable to create stat [%s]", __FUNCTION__, Stats::STAT_NAMES[i]);
- }
+ }*/
}
}
}
-void Stats::increment(Stats::STAT st, TSMgmtInt step /* = 1 */) {
+//FIXME step should be TSMgmtInt but for some reason the linker is having some strange int vs long name mangling issue.
+void increment(Stats::STAT st, int step/* = 1 */) {
if (g_system) {
+//FIXME doesn't return avalue.
+g_system->increment(st, step);
+/*
if (!g_system->increment(st, step)) {
Utils::ERROR_LOG("[%s] Unable to increment stat [%s] by step [%d]", __FUNCTION__, step,
Stats::STAT_NAMES[st]);
}
+*/
}
}
+}}
diff --git a/esi/lib/Stats.h b/esi/lib/Stats.h
index f492553..d2036bc 100644
--- a/esi/lib/Stats.h
+++ b/esi/lib/Stats.h
@@ -34,7 +34,8 @@ namespace EsiLib {
class StatSystem {
public:
virtual void create(int handle) = 0;
- virtual void increment(int handle, TSMgmtInt step = 1) = 0;
+//FIXME step should be TSMgmtInt
+ virtual void increment(int handle, int step = 1) = 0;
virtual ~StatSystem() { };
};
diff --git a/esi/lib/Utils.h b/esi/lib/Utils.h
index d5803e3..6308e7c 100644
--- a/esi/lib/Utils.h
+++ b/esi/lib/Utils.h
@@ -28,6 +28,7 @@
#include "DocNode.h"
#include "ComponentBase.h"
+#include <cstring>
#include <string>
#include <map>
#include <list>
diff --git a/esi/lib/Variables.h b/esi/lib/Variables.h
index 82d40a2..7bc985c 100644
--- a/esi/lib/Variables.h
+++ b/esi/lib/Variables.h
@@ -28,6 +28,7 @@
#include <list>
#include <boost/noncopyable.hpp>
+#include <cstring>
#include "ComponentBase.h"
#include "StringHash.h"
#include "HttpHeader.h"
diff --git a/esi/plugin.cc b/esi/plugin.cc
index 727749d..a83903c 100644
--- a/esi/plugin.cc
+++ b/esi/plugin.cc
@@ -119,7 +119,8 @@ public:
void create(int handle) {
g_stat_indices[handle]=TSStatCreate(Stats::STAT_NAMES[handle], TS_RECORDDATATYPE_INT, TS_STAT_PERSISTENT, TS_STAT_SYNC_COUNT);
}
- void increment(int handle, TSMgmtInt step = 1) {
+// void increment(int handle, TSMgmtInt step = 1) {
+ void increment(int handle, int step = 1) {
TSStatIntIncrement(g_stat_indices[handle], step);
}
};
@@ -226,7 +227,7 @@ void
ContData::getClientState() {
TSMBuffer req_bufp;
TSMLoc req_hdr_loc;
- if (TSHttpTxnClientReqGet(txnp, &req_bufp, &req_hdr_loc) == 0) {
+ if (TSHttpTxnClientReqGet(txnp, &req_bufp, &req_hdr_loc) != TS_SUCCESS) {
TSError("[%s] Error while retrieving client request", __FUNCTION__);
return;
}
@@ -242,12 +243,18 @@ ContData::getClientState() {
}
if (req_bufp && req_hdr_loc) {
TSMLoc url_loc;
- TSHttpHdrUrlGet(req_bufp, req_hdr_loc, &url_loc);
+ if(TSHttpHdrUrlGet(req_bufp, req_hdr_loc, &url_loc) != TS_SUCCESS) {
+ TSError("[%s] Error while retrieving hdr url", __FUNCTION__);
+/*FIXME Does this leak?*/
+ return;
+ }
if (url_loc) {
if (request_url) {
TSfree(request_url);
}
- request_url = TSUrlStringGet(req_bufp, url_loc, NULL);
+//FIXME TSUrlStringGet says it can accept a null length but it lies.
+ int length;
+ request_url = TSUrlStringGet(req_bufp, url_loc, &length);
TSDebug(DEBUG_TAG, "[%s] Got request URL [%s]", __FUNCTION__, request_url ? request_url : "(null)");
int query_len;
const char *query = TSUrlHttpQueryGet(req_bufp, url_loc, &query_len);
@@ -302,7 +309,8 @@ ContData::getServerState() {
TSMLoc hdr_loc;
if (!TSHttpTxnServerRespGet(txnp, &bufp, &hdr_loc)) {
TSDebug(DEBUG_TAG, "[%s] Could not get server response; Assuming cache object", __FUNCTION__);
- input_type = DATA_TYPE_PACKED_ESI;
+//FIXME In theory it should be DATA_TYPE_PACKED_ESI but that doesn't work. Forcing to RAW_ESI for now.
+ input_type = DATA_TYPE_RAW_ESI;
return;
}
if (checkHeaderValue(bufp, hdr_loc, TS_MIME_FIELD_CONTENT_ENCODING,
@@ -513,10 +521,12 @@ transformData(TSCont contp)
consumed += data_len;
block = TSIOBufferBlockNext(block);
+/*FIXME this chunk of code looks to be in error. Commenting out.
if (!block) {
TSError("[%s] Error while getting block from ioreader", __FUNCTION__);
return 0;
}
+*/
}
}
TSDebug((cont_data->debug_tag).c_str(), "[%s] Consumed %d bytes from upstream VC",
@@ -799,7 +809,7 @@ modifyResponseHeader(TSCont contp, TSEvent event, void *edata) {
}
TSMBuffer bufp;
TSMLoc hdr_loc;
- if (TSHttpTxnClientRespGet(txnp, &bufp, &hdr_loc)) {
+ if (TSHttpTxnClientRespGet(txnp, &bufp, &hdr_loc) == TS_SUCCESS) {
int n_mime_headers = TSMimeHdrFieldsCount(bufp, hdr_loc);
TSMLoc field_loc;
const char *name, *value;
@@ -916,7 +926,7 @@ static void
maskOsCacheHeaders(TSHttpTxn txnp) {
TSMBuffer bufp;
TSMLoc hdr_loc;
- if (TSHttpTxnServerRespGet(txnp, &bufp, &hdr_loc) == 0) {
+ if (TSHttpTxnServerRespGet(txnp, &bufp, &hdr_loc) != TS_SUCCESS) {
TSError("[%s] Couldn't get server response from txn", __FUNCTION__);
return;
}
@@ -984,7 +994,7 @@ isTxnTransformable(TSHttpTxn txnp, bool is_cache_txn) {
header_obtained = is_cache_txn ? TSHttpTxnCachedRespGet(txnp, &bufp, &hdr_loc) :
TSHttpTxnServerRespGet(txnp, &bufp, &hdr_loc);
- if (header_obtained == 0) {
+ if (header_obtained != TS_SUCCESS) {
TSError("[%s] Couldn't get txn header", __FUNCTION__);
goto lReturn;
}
@@ -1055,7 +1065,7 @@ isInterceptRequest(TSHttpTxn txnp) {
TSMBuffer bufp;
TSMLoc hdr_loc;
- if (!TSHttpTxnClientReqGet(txnp, &bufp, &hdr_loc)) {
+ if (TSHttpTxnClientReqGet(txnp, &bufp, &hdr_loc) != TS_SUCCESS) {
TSError("[%s] Could not get client request", __FUNCTION__);
return false;
}
