It now bails out due to a workspace overflow which it acually did
trigger all the time, but that remained unnoticed due to #3194

The test to multiply the cookie header 128 times (8x in the first
regex, 16x in the second) was not realistic even for "modern times",
so I do not think we should have workspaces sized by such an example.

Since 8baf4a69 we lost information about
an overflowed workspace by calling std.log(), which resulted from use of
the workspace for construction of a contiguous string from the argument
constituents.

Since then, we have changed the interface to STRANDS, but this issue
remained.

We now solve the case for real by pushing the string concatenation down
to VSL: New versions of VSL and VSLb (coded by example of VSLv() and
VSLbt()) take a strands argument and create a log record without
additional copy overhead.

These solve #3194 for std.log(), make logging more efficient and, in
particular, allow use of std.log() in low workspace conditions (because
they do not require any).

Also improve test coverage for std.log()

Ref #3194

note: no change to vrt.h, this is a private interface between vcl and
vrt_vcl/vpi.

... potentially saving a lock/unlock on the critical vcl_mtx

Both callers of vcl_get(), VPI_vcl_select() and VCL_Refresh(),
potentially have to unref another vcl first.

For VCL_Refresh(), this happens when the active vcl changes and the
previously active vcl is cached in (struct worker).vcl, which can be
assumed to always be the case on a busy system.

For VPI_vcl_select(), this happens when switching vcls at
esi_level > 0.

To summarize, this patch will primarily reduce contention on the
vcl_mtx after a 'vcl.use' CLI command on a busy system and for
'return(vcl(...))' with ESI.

This is already implied in the code (because otherwise we would
potentially leak a vcl reference (busy counter)).

(yes, I have run make check with this)

We should not consider an early close of the backend connection a
failure of the test, this vtc is not concerned with bodies.

This tests that we are at least consistent, though not necessarily
correct

See #2902

Our code was inconsistent with respect to the cli vrt_ctx.msg being set
for vcl temperature events.

This patch is intended as a temporary bandaid to at least make the code
consistent.

A proper solution is pending after clarification of the intent in #2902,
in particular wrt VCL_EVENT_COLD, which, by design, should not have a
ctx->msg, but does have one at the moment.

See #2902 for more details

we were missing VCL_TaskEnter()/VCL_TaskLeave()

Noticed working on #2902

it is the true (1) argument to VCL_Get_CliCtx which creates the msg vsb.

Fixes	#1853

... which was received from martin in private communication, thank you

Currently, with the 505b7bd9 patch, when
calling WS_ReserveSize with bytes equal to the amount of workspace that is
currently available, it will return zero and mark overflow.

This patch redoes the patch, and changes it to return zero and overflow
only when the requested number of bytes is larger than what is available.

The test is now conducted on the session workspace in order to have the
client workspace free to generate vtc.log() output which needs
workspace to format the string.

This made me notice that vtc.log, via WS_Reset(), clears a previous
workspace overflow and I wonder if this is how it should be.

introduced with a74315bc

spotted by coverity

... introduced with 3bb8b84c:

in Pool_Work_Thread(), we could break out of the for (i = 0; i <
TASK_QUEUE__END; i++) loop with tp set to the value from the previous
iteration of the top while() loop where if should have been NULL (for no
task found).

Noticed staring at #3192 - unclear yet if related

When we turned the vcl temperatures into a struct, we overlooked this
statement because of the cast.

Noticed when staring at #3192

in these cases, the initial AN(objp) was missing, so we would
potentially dereference a NULL pointer without an explicit assertion
failure.

in VCL_Rel(), AN(*vcc) was likely just a typo resulting in a semantic
noop (the check was a duplication of the NULL check in
CHECK_OBJ_NOTNULL).

... more examples why using the macro is a good idea

command:

spatch -I include/ -I bin/varnishd/ --dir . --in-place \
	--sp-file tools/coccinelle/take_obj_notnull.cocci

The previous stabilization turned out not to reliably work with FreeBSD
on aarch64 hardware. It was still an improvement overall, but the test
case turned out to be a bit cryptic and when [idle_]send_timeout support
landed in VCL it wasn't obvious how to add coverage.

This attempt bites the bullet and defines one pair of client/logexpect
instances per use case and clearly (I hope) indicates why it is doing
things or not doing them.

Since we now have two clients that aren't expected to complete before
the test case itself the server is in dispatch mode instead of repeating
its scenario. Using barriers in dispatch mode should raise a red flag
for any reviewer, but in this case the barriers outside the server are
properly serialized, are systematically used by every single client, and
as a result should be safe.

As usual, personal testing limited to x86_64 Linux and FreeBSD.

when we stall sending on the backend side, we _have_ to run into a
timeout on the client side, haven't we?

and enable debug output for better failure analysis

Furhter lighten the load for the test to succeed on FreeBSD vtest,
hopefully.

Also, when stars align, we do not git the busy object (streaming) and do
not see chunked encoding.

we need a way for vtc to reach into session setup I guess

freebsd vtest boxes cannot complete the c2 response within the 20s
extended send_timeout, apparently for too much overhead from timeout
handling.

Try to fix with less pressure

02bc0a68 requires 24 bytes more on
64bit, which brought o00005.vtc just over the top on smartos 64bit
machines (the vtc tests PROXY TLVs, which get stored on the session
workspace).

Increase the default workspace_session by 256bytes (50%) to accomodate
sane proxy TLV lengths by default.

This is about 10x more than the minimum increase sufficient to
accomodate the increased workspace usage. I went for 768 bytes because

- this gives us the next least common multiple (12k) with the common 4K
  page size unless we went up to 1k

- we were already quite tight with the default and could not accomodate
  common proxy TLV use (see for example #3131)

As a ballpart reality check, I do not think that spending an additional
256MB should be an issue for users supporting 1M sessions.

and test that they work for H1

To @Dridi, I would appreciate if you could have another look at s10.vtc.
I *tried* to keep your good ideas intact, and hope you can live with
these changes.

since d4b6228e the busyobj might also
gain one, so depending on who deref's first, there may be one or zero
references left.

thank you!

In the absence of a core dump, we do not have any information yet in the
panic output about vcl object instances, for example to find out which
object a priv belongs to when the instance address is used for
per-instance priv state.

To make this information available at the time of a panic, we add the
following:

* A struct vrt_ii (for instance info), of which a static gets
  filled in by VCC to contain the pointers to the C global variable
  instance pointers at VCC time

* A pointer to this struct from the VCL_conf to make it available to
  the varnishd worker

* dumps of the instance info for panics

This gives the receiver of the PROXY header (usually the ssl-onloader)
the opportunity to set the SNI (HostName field) from the TLV value, for
the TLS handshake with the remote backend.

From
https://github.com/nigoroll/varnish-cache/commit/e0eb7d0a9c65cdc3c58978656b4c71f4ab8aabca
edited by @nigoroll to split out the proxy header functionality.

Add vmod_debug access to the proxy header formatting and test it