Fix director list membership and temperature events

[nigoroll]

this PR fixes #3094

We add missing locking for operations on the director list to ensure consistency with concurrent dynamic director additions and deletions.
We remove the temperature lock which prevented director deletions during COLD events and ensure that only backends on the director list receive events.

Please note that there is no explicit vtc, the test case has been added to vmod_debug and runs with every invocation of that vmod.

Please consider the individual commit messages for additional detail on the reasoning.

[dridi]

I'm not sure why we need 22e44fe looking at everything else, wherever you use the temperature on its own it comes from a VCL.

Otherwise, I'm happy to see the temperature rwlock go away and overall this looks good to me. I will look closer.

[dridi]

I think this is generally a good opportunity to get rid of the VCL_(COLD|WARM) macros too:

• turn VCL_TEMP_* into structs
• give them .cold and .warm fields
• s/VCL_COLD(x)/(x)->cold/
• s/VCL_WARN(x)/(x)->warm/

[dridi]

See previous comments.

[nigoroll]

@dridi with all due respect, I think your suggestion is overkill. Accessing a field in a struct is by no means more efficient than two pointer comparisons (to compile time values), the suggestion does not contribute to the fix and I do not see how else it would be useful.
If you really think this would be needed, can we discuss it in a separate PR?

[nigoroll]

I'm not sure why we need 22e44fe looking at everything else, wherever you use the temperature on its own it comes from a VCL.

this is used here: the temperature is saved inside the lock for consistency, then checked outside

varnish-cache/bin/varnishd/cache/cache_vrt_vcl.c

Line 204 in be71fd8

if (!VCL_WARM(temp) && temp != VCL_TEMP_INIT)

[dridi]

@dridi with all due respect, I think your suggestion is overkill.

I discussed it a long time ago and @bsdphk wants to move away from const char * compile-time constants for better type safety. It happened in other parts of the code base, but never here.

Accessing a field in a struct is by no means more efficient than two pointer comparisons (to compile time values), the suggestion does not contribute to the fix and I do not see how else it would be useful.

As explained above, this is not about efficiency, on this account I agree with you.

If you really think this would be needed, can we discuss it in a separate PR?

Like I said, now is a good opportunity to make this change since VCL temperature gets some scrutiny. I think this can be pushed directly to master once this is merged, I will do it.

[nigoroll]

@dridi thank you for the explanation. The type safety argument makes sense.