PRIV_CALL, VRT_re_init, and mutexes

[Guillaume Quintard]

Hi, and welcome!

In this case, priv comes from PRIV_CALL, meaning it's going to be the same
for all the request/vcl that call the function from the same spot. This is
done to cache the regex, so you only compile for the first call.

However, I'm thinking that you may have the same priv with a different
string, and then everything goes to hell. Could anyone familiar with
PRIV_CALL confirm?

Cheers,

-- 
Guillaume Quintard

On Fri, Apr 27, 2018 at 8:05 AM, Stephen J. Butler <stephen.butler at gmail.com
> wrote:

> I'm a new developer/user for Varnish, and was looking to extend a module
> with some regex support (vmod_cookie if you must know). I don't know
> anything about vmod development or Varnish threading, so to get an idea of
> how to do this I looked at the vmod_header module.
>
> But I'm confused about why they thought they needed a mutex to protected a
> call to VRT_re_init. Is this just a useless, historic thing that got left
> in for some reason?
>
> https://github.com/varnish/varnish-modules/blob/master/src/vmod_header.c
>
> If you look at vmod_remove it has a 2nd parameter that's in the vcc as
> PRIV_CALL. This parameter is initialized with the string value from the
> last parameter (a regex pattern). It does this by calling header_init_re().
> And if you look at that function you see:
>
> /*
> * Initialize the regex *s on priv, if it hasn't already been done.
> * XXX: We have to recheck the condition after grabbing the lock to avoid a
> * XXX: race condition.
> */
> static void
> header_init_re(struct vmod_priv *priv, const char *s)
> {
> if (priv->priv == NULL) {
> assert(pthread_mutex_lock(&header_mutex) == 0);
> if (priv->priv == NULL) {
> VRT_re_init(&priv->priv, s);
> priv->free = VRT_re_fini;
> }
> pthread_mutex_unlock(&header_mutex);
> }
> }
> My reading of the docs makes me think that this "priv" only lives for each
> single call of the function. There should be no reason to protect it with a
> global mutex. Do the internals of VRT_re_init() require this? I can't
> imagine how, because if it did then this method of protecting it is broken
> anyway.
>
> I suspect this is leftover from some rewrite or updating of the module but
> wanted to check.
>
>
> _______________________________________________
> varnish-dev mailing list
> varnish-dev at varnish-cache.org
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20180427/ba439f5b/attachment.html>