vca: Promote shut_mtx to instrumented lock

[dridi]

This would have been a cherry-pick of a commit from #3959, but #3976 introduced many conflicts so I had to reapply it manually.

This port was motivated by the curious inclusion of pthread.h that should have happened via a higher-level include in the cache process. Either from cache.h, or indirectly from cache_varnishd.h for internal subsystems.

Refs 119832d5e964b01dd672ef6e15f4e2c128bd0c4e

It turns out this include is needed because cache_acceptor.h is included from mgt code, which looks like an improvement waiting to happen in that area. The mgt process is single-threaded, so we should really treat the inclusion of pthread.h as a red flag.

Switching to an instrumented lock means that mgt code can ignore an opaque struct lock pointer passed to the update() callback, on top of exposing metrics regarding this lock.

The encapsulation problem was already registered in an XXX comment in mgt_acceptor.c, and despite this modest improvement, restoring proper mgt/cache boundaries in the listen socket department will require more work.

---

Another possibility to remove the reference to the shutdown lock altogether and keep it strictly internal again, would be to extract a new can_update() callback from the current update(), and make cache_acceptor.c lock around the update() call.

I'm open to either making that a subsequent change, or the first step in the pthread.h removal from mgt code. Ideally, I'd like to add a build-time check that prevents pthread.h from being included at all in mgt code in general.

Of course, the first thing to do is attempt a build of this branch on OpenBSD. Maybe @catap could help.

[catap]

@dridi sure.

I've tried local root 1e65656da4dfc41dea3efc22582b2ffcf5b69ac2 on OpenBSD-current with autoconf-2.72 and automake-1.17. I had run just ./configure and make. It passed.

Let me know if you need something else.

[bsdphk]

This looks like a good compromise until we move the acceptor code.

Ok with me.

[dridi]

Ideally, I'd like to add a build-time check that prevents pthread.h from being included at all in mgt code in general.

I spent 5m on this and gave up, I registered my observations in 67bc0aee4b0531350217e438886b8730c6805c16.

A possibility would be to link varnishd from new libvarnishd-mgt.a and libvarnishd-cache.a binaries. This way we could add bin/varnishd/mgt to the include path for libvarnishd-mgt.a and create a new bin/varnishd/mgt/pthread.h file with nothing but an #error directive.

Thoughts?

[asadsa92]

How about:

$ grep _PTHREAD_H /usr/include/pthread.h
#ifndef _PTHREAD_H
#define _PTHREAD_H	1

#ifdef _PTHREAD_H
#error "Err"     
#endif           

Not sure how portable this is...

EDIT: nvm.

[dridi]

With cache_acceptor.h including pthread.h, but being included after mgt.h, you wouldn't catch it.

See https://github.com/varnishcache/varnish-cache/commit/67bc0aee4b0531350217e438886b8730c6805c16.

[asadsa92]

How about this?

diff --git a/bin/varnishd/mgt/mgt.h b/bin/varnishd/mgt/mgt.h
index 2610aaf75..84fb22e6b 100644
--- a/bin/varnishd/mgt/mgt.h
+++ b/bin/varnishd/mgt/mgt.h
@@ -251,6 +251,11 @@ extern const char *mgt_vmod_path;
 #error "Keep pthreads out of in manager process"
 #endif
 
+static inline int pthread_create(void)
+{
+       return (0);
+}
+
 #define MGT_FEATURE(x)         COM_FEATURE(mgt_param.feature_bits, x)
 #define MGT_EXPERIMENT(x)      COM_EXPERIMENT(mgt_param.experimental_bits, x)
 #define MGT_DO_DEBUG(x)                COM_DO_DEBUG(mgt_param.debug_bits, x)
diff --git a/bin/varnishd/mgt/mgt_child.c b/bin/varnishd/mgt/mgt_child.c
index ae08c54e6..9a83f3abc 100644
--- a/bin/varnishd/mgt/mgt_child.c
+++ b/bin/varnishd/mgt/mgt_child.c
@@ -61,6 +61,8 @@
 
 #include "common/heritage.h"
 
+#include <pthread.h>
+
 static pid_t           child_pid = -1;
 
 static struct vbitmap  *fd_map;

Then, we get this compilation error (if we remove the artificial added include, it compiles fine):

In file included from mgt/mgt_child.c:64:
/usr/include/pthread.h:202:12: error: conflicting types for 'pthread_create'
extern int pthread_create (pthread_t *__restrict __newthread,
           ^
mgt/mgt.h:254:19: note: previous definition is here
static inline int pthread_create(void)
                  ^
1 error generated.

[dridi]

That's a neat trick!

[dridi]

I would actually do something like this:

static inline void
pthread_create(void)
{

        WRONG("Intentional conflict with pthread.h");
}

But yes, that should work.

[dridi]

Here goes nothing: #4350