Quote more. Do not quote too much

Ref da2409c3

We have now seen a second case after
184736fa, and a simple internal rename
from bool to boolean does the job.

Should have no user-visible impact.

In fee476ac VRB_Ignore(), VRB_Cache()
and VRB_Free() were rightly out of vmods' sight.

But VRB_Iterate() is legitemately required for vmods, and including it
in the move seems to be an accident.

Advise from shellcheck taken undogmatically.

(I always forget: is $() for `..` posix-sh or not?)

information to make them easier to clone.

Salvaged from #3503

The rationale is that if we add another type of task in the future, for
example probe tasks, we won't need to update the default mask for VMOD
functions and object methods.

Refs #3498

With a default read mask based on the type of symbol. Starting with a
default comprehensive mask should then help implement the $Restrict
stanza from VIP4, simply by AND-ing the masks.

Simply take an ssize_t and return size_t asserting that the value is
greater than or equal to zero.

The name might still not be optimimal, as well as choice of *size_t.

Addresses Coverity CID 1472401, CID 1472402

This way every failure can be tested again without selecting a specific
subdirectory.

adds a variant of PAN_dump_struct() which does not start another
indentation level on a new line.

A copy-pasta in 55dffc35 prevented the
top request from being dumped.

55dffc35 introduced a PAN_dump_struct()
call, which implies printing '{' and VSB_indent(vsb, 2). Fix missing '}'
and wrong indentation.

Also remove redundant "priv" output, which now happens in PAN_dump_struct()

Diff best viewed ignoring whitespace (-b option).

This test fails constantly now on vtest arm / Freebsd 13.0 for timeout.

As vcl compilations are probably the most expensive operations of vtcs,
we speed up this test by reducing the number of vcls loaded from 19 to
11. We do this by collapsing all of the client side failure tests into a
single vcl. Being at it, we also reduce logexpect synchronization points.

The number of vcl loads could be reduced further by also collapsing the
backend side, but as this would reduce clarity more than for the client
side alone, I first wanted to see if this first step brings down tuntime
enough.

Do not overwrite an already latched errno.

Note that it does not make any difference at the moment because we only
ever use ENOMEM

Ref #3502

This is similar to #3502, but the out-of-storage condition happens after
the object has been created successfully.

Use a logexpect to ensure we test as intended.

We set up fetch filters (VFPs) before initializing a storage object to
fetch into. If that fails, we close the filters again.

The esi_gzip VFP interacts with the storage object and was missing
error handling for the case of a teardown with no storage object.

Implementation:

We make sure that for the VFP_ERROR case, we do not interact with the
storage object (which makes no sense if that is going to be C-4'ed any
moment).

vfp_vep_callback() keeps its hands off if (struct vef_priv).error is
set, so we use that to avoid complications in the code.

... which is not important enough for me to tackle it right now, in
particular because I really do not understand enough about how this code
works yet, e.g. which of the if() blocks can possibly happen in which
order. *cough* fsm? *cough*

This was already the behavior before #3468, but then the filtering was
moved to libvarnishapi to prevent varnishncsa from duplicating the logic
behind transaction collection.

Unfortunately, despite libvarnishapi's effort to not consider a client or
backend transaction without a Begin tag as a candidate, a transaction
missing one will get a synthetic Begin prior to dispatch.

Regardless, that would have only applied to vxid grouping so even if
varnishncsa was presented by default with only relevant transactions, it
could still see incomplete transactions in request grouping panic for
the same reason.

Fixes #3501

do someone a favor and reformat the VPI_vcl_fini() call.

Because ifp->fin basically gets output via "\t%s\n", this triggers my
OCD in a some other way ;), but I am not going down that rabbit hole
at this point...

We do not need to call trunc() to truncate a double into an integert.

Mark the intention for flexelint with a cast.

This change is based on the precondition that VRT_fail() should work
wherever we have a VRT_CTX and DTRT, see also previous.

on vcl_init {}
--------------

We already handled a "direct" VRT_fail() via a vmod function
correctly.  This commit adds a test.

Yet we can also VRT_fail() via a PRIV_TASK fini callback and did not
handle that case (triggered an assertion in VRT_fail()). Notice that
this case has worked in client / backend tasks for long and a test
case has been added with 746384b2 . It
has now gained relevance with 43d9e5fb

on vcl_fini {}
--------------

By design, vcl_fini {} must not fail, which is also why the only valid
return() value is "ok" (VCL_RET_OK).

Consequently, we had

	if (method && *ctx->handling && *ctx->handling != VCL_RET_OK) {
		assert(ev == VCL_EVENT_LOAD);

in vcl_send_event().

And then came VRT_fail().

If we wanted to sustain an assertion similar to the above, we would
need to require vmod authors to never call VRT_fail() from
VCL_MET_FINI - which would complicate code, be likely overlooked and
receive little to no test coverage.

So instead we now ignore errors during vcl_fini {} and emit a
VCL_error log line.

I considered the alternative to check for VCL_MET_FINI in VRT_fail(),
but decided to handle the exception where it applies, which is in
vcl_send_event().

I am aware that this part of the change may trigger some controversy
and I am open to discussing alternatives. If anything, we now avoid
unmotivated assertion failures triggered for the new tests in
v00051.vtc for the time being.

Ref 43d9e5fb :

PRIV_* fini methods need to leave (struct vrt_ctx).handling alone,
except that they might call VRT_fail(), see also
746384b2

Thus we add assertions that handling be either 0 or VCL_RET_FAIL
outside the FSM.

To be able to do so, we need to change VCL_RET_OK into 0 when
vcl_init{} has returned successfully.

The vcl_fini{} case is slightly more complicated:

By design, only "ok" (VCL_RET_OK) is allowed, but VRT_fail() also
added VCL_RET_FAIL, so we de-facto get a "fail" return if any vmod
code called VRT_fail().

Because PRIV_* handling happens from VCC generated code via
VGC_Discard(), we need to check and change (struct vrt_ctx).handling
right after calling vcl_fini{} / VGC_function_vcl_fini() from
VGC_Discard(). This is VPI_vcl_fini().

Implementation note:

I also considered void VPI_vcl_fini(VRT_CTX, vcl_func_f fini_sub),
having VPI_vcl_fini call the fini_sub, but that stirred up includes of
VPI where vcl.h is not included.

Though introduced for the purpose of failing upon rollback, they
actually just call VRT_fail() from the PRIV_* fini callback and thus
should be named accordingly.

Not sure where things went wrong on this...

Only if strtoll(3) stumbled upon a VNUM special character.

Fixes #3463

forgotten static declaration

The time_t t variable is not guaranteed to have a specific size, so
instead we use a dedicated long l variable. The explicit cast from
double is also restored, spotted by flexelint.

Refs #3499

The option was present in libvarnishapi but never exposed.

And remove [bcE]_opt handling entirely out of varnishncsa, relying on
libvarnishapi to do this filtering. As such we can consolidate the tag
management without backend marker fiddling.

This doesn't change the behavior of varnishncsa that already ignores ESI
sub-requests unless the -E option is specified. This is however breaking
the default behavior of other VSL processing tools that gained the -E
option, aligning with varnishncsa's behavior. Except varnishhist, that
gained the E filter in its -P argument instead of the -E option.

This allows to also not collect ESI transactions in VXID mode to further
reduce churn and reduce the risk for overruns in varnishncsa.

But paced, because they may repeat in a semi-tight loop.