search

varnish / varnish-modules

Collection of Varnish Cache modules (vmods) by Varnish Software
Other
182 stars 86 forks source link

DO NOT MERGE: Add vmod_bodyaccess #69

Closed aondio closed 7 years ago

aondio commented 7 years ago

Hi everyone, this PR acts as reminder, so we can discuss this on Monday during our internal meeting.

For the moment let's only consider Varnish Cache. As we discussed last week the intent of this PR is to add vmod_bodyacces to varnish-modules. As of today we want varnish-modules to build against VC 4.1/5.x.

PROBLEM:

This PR build successfully against VC 4.1, but not against 5.x.

One of the function used to iterate over the request body is VRB_Iterate and in Varnish Cache 4.1 that's its prototype: _ssize_t VRB_Iterate(struct req req, req_body_iter_f func, void priv);_ whereas the prototype for "req_body_iter_f" is: _typedef int (req_body_iter_f)(struct req , void priv, void ptr, sizet);

Starting from VC 5.0 VRB_Iterate changed to: _ssize_t VRB_Iterate(struct req , objiterate_f func, void priv);_ where objiterate_f is: _typedef int objiterate_f(void priv, int flush, const void *ptr, ssizet len);

This obviously means that the same branch will build only against VC4.1.

POSSIBLE SOLUTION:

We want a single branch to build against VC4.1/5.x. My suggested solution is:

  1. we detect in configure.ac the prototype of VRB_Iterate and define a macro which says if it is the function used in VC4.1 or if it is the function from VC5.x
  2. in vmod_bodyaccess.c we use the previously mentioned macro to define which part of code should be used and when

Is this the BEST way to solve this problem? If it is, I'd like to have some input on point 1.

Looking forward to your ideas on Monday! :)

nigoroll commented 7 years ago

have not checked the differences but I'm using https://github.com/nigoroll/libvmod-bodyaccess

dridi commented 7 years ago

As discussed during the aforementioned Monday we cannot AFAIK detect changes in the prototype of VRB_Iterate. We can however either detect the req_body_iter_f and objiterate_f prototypes or use the Varnish version for the #ifdef dance.

dridi commented 7 years ago

@aondio please have a look at 9854867, it compiles fine for me with 4.1 and fails just fine with 5.0 :)

See the TODO comments where I suppose you would add the 5.x alternative code.

dridi commented 7 years ago

In its current state the PR passes on my machine with Varnish 4.1.5 and 5.0.0 and apparently on Travis CI too. I'm not done with testing and polishing but feel free to have a look anyway. Please don't merge it yet.

Comments welcome regarding the handling of the test suite.

dridi commented 7 years ago

I think it can be merged now, please review.

dridi commented 7 years ago

tests/bodyaccess/5x/test04.vtc fails with Varnish 5.1.2 so I'll investigate that before it can be reviewed.

dridi commented 7 years ago

It turns out the failure with 5.1.2 was a logic error in the test case. I have now fixed it and after I realized that enabling Hash log records didn't show anything I tweaked the test case further and made the VMOD log what it's hashing.

This is again ready for a review.

rezan commented 7 years ago

Looks good to me

dridi commented 7 years ago

I'm merging this, and I will try to improve it further with a future pull request.

Thanks for the review!