Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7.1 #8

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

7.1 #8

wants to merge 9 commits into from

Conversation

nigoroll
Copy link
Contributor

@nigoroll nigoroll commented May 20, 2022

This series of changes makes the vmod work with varnish-cache post-7.1 (master as of varnishcache/varnish-cache@142c3a0)
Much more could be done, consider this PR as a first "time to market" patch series

Also includes #7 which might be justified should this PR not be accepted

Vmods are banned from cache_param for quite some time now, so the
vmod should have its own parameter for the gzip buffer size.
Much more can be done here to reduce the required varnish internal
headers, but for now this commit concludes the required changes for
varnish-cache master as of now (post 7.1)
varnish-cache serializes the vmod events, so, unless the vmod itself has
any concurent access, no mutex is required.
nigoroll added a commit to varnishcache/homepage that referenced this pull request May 23, 2022
The PR to bring it up to speed is not yet merged, but I hope that will
change soon

Refinitiv/libvmod-soap#8
Copy link
Member

@JonathanHuot JonathanHuot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does these changes are also compatible with Varnish4 or does it require explicit versioning to separate V4 with V7 ?


#define POOL_KEY "VRN_IH_PK"

static pthread_mutex_t soap_mutex = PTHREAD_MUTEX_INITIALIZER;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the rationale behind this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see 47ccd3e

varnish-cache serializes the vmod events, so, unless the vmod itself has
any concurent access, no mutex is required.

{
struct priv_soap_task *soap_task;

AN(priv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we preserve the AN(priv) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAST_OBJ_NOTNULL() two lines down already asserts on the value not being NULL

@nigoroll
Copy link
Contributor Author

Does these changes are also compatible with Varnish4 or does it require explicit versioning to separate V4 with V7 ?

Some changes are incompatible in varnish-cache. There are some vmods which try to retain backwards compatibility using macros, wrappers etc. but I would recommend against this practice and just branch off the current code as, say, a 5.x branch and continue going forwards with master.
But really it is up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants