I did not understand when I committed
119b4174 that the local variable was
required for flexelint to unterstand that this is not an out-of-bounds
access.

This quote of phk from the top level rant sais it all:

 * Do I need to tell you that static code analysis tools have a
 * really hard time coping with this, and that they give a lot of
 * false negatives which confuse people ?

So true. Sorry for going a smaller but almost full circle here, I
started with good intentions and now all that's left is the desire to
leave the code at least a little cleaner as I found it.

I would have preferred avoiding the duplication of the suckaddr member
types, but flexelint spits "Warning 550: Symbol 'sua' (line 229) not
accessed"

vtest has seen a case where other workspace debug messages interfered.

FTR, it would be nice if we could backref a prior expect in order to
expect the same workspace address

Across the platforms we support, the overhead of stevedore allocations
varies slightly due to different sizes of our structs.

const char *const bindings_help[] = {
varnishstat_curses_help.c  9  Note 9075: external symbol 'bindings_help'
    defined without a prior declaration
                            _
const int bindings_help_len = 65;
varnishstat_curses_help.c  78  Note 9075: external symbol
'bindings_help_len'
    defined without a prior declaration

Could be that there is the bereq # printed right next to the client |
as in this vtest result:

**   top  === process p1 -expect-text 22 0 { | }
**** dT   3.593
**** v1   vsl|          0 CLI             - Rd ping
**** v1   vsl|          0 CLI             - Wr 200 19 PONG 1585796425 1.0
**** dT   3.806
**** p1   stdout|\x1b[1;10H2\x1b[22;24H|#

when the second request did not happen within the default
backend_remote_error_holddown == 0.25s, we would see two ECONNREFUSED
instead of one and a holddown.

Seen in vtest:

**   top  === varnish v1 -expect VBE.vcl1.foo.fail_econnrefused > 0
**** dT   6.132
**   v1   as expected: VBE.vcl1.foo.fail_econnrefused (2) > 0
**   top  === varnish v1 -expect VBE.vcl1.foo.helddown > 0

Flexelint sput warning 429 that the malloc() return value was neither
freed not returned.

This simplification makes it clear. The assertion on malloc() having
succeeded is in VSA_Build()

Ref #3275

This ticks off an XXX comment: We basically assert on malloc to succeed
everywhere, so we should stick with it here, too.

Closes #3275

Fixes: #3273

Yes, we do not want to rely on timing, but I do not see how we could
test the lru_interval timer otherwise

seen in vtc:

---- v1   Not true: MAIN.n_lru_moved (0) == 1 (1)
*    top  RESETTING after ../../../../bin/varnishtest/tests/r02527.vtc
**   s1   Waiting for server (3/-1)
**   v1   Wait

That is, when the struct is embedded in another struct. This is a
mechanical change allowing to always treat tasks like pointers.

See also previous commit:

With this parameter set to on, any ACL entries in non-canonical form
cause a VCL compilation error rather than only a warning.

Summary:

ACL entries with netmasks shorter than the maximum for the respective
protocol represent network addresses and as such, by convention,
should be written with all zero bits in the host part to avoid
confusion.

This patch adds VCL compile warnings and improved logging if they are
not.

Discussion:

For example, while 1.2.3.0/24 and 1.2.3.255/24, in CIDR notation, both
specify all addresses with the first three octets matching 1, 2 and 3,
using the latter can be a source of subtle confusion.

This becomes particularly apparent with netmasks outside byte
boundaries: 1.2.6.0/22 specifies addresses 1.2.4.0 - 1.2.7.255, but
not so experienced administrators might be tempted to think that it
specified 1.2.6.0 - 1.2.9.255.

To summarize, denoting network addresses in non-canonical form is
confusing, a possible source of error and additionally complicates
analyses.

This patch makes sure that such mishaps do not remain unnoticed by

- issuing warnings during VCL compilation about non-canonical network
  addresses

- Logging ACL matches together with the canonical address

The actual matching code is not touched, but a minor simplification
can be applied later.

Follow the spirit of the vsa.c top level rant and spare callers the
hassle of creating sockaddrs specific to ip4/ip6 just to build a VSA,
which is intended to avoid having to special-case the protocols in the
first place.

In #3154 we said we would add this to the upcoming VSA_BuildFAP(),
but actually VSA_Build() is the right place.

- Centralize duplicated code in sua_len()
- Have VSA_Malloc call VSA_Build
- Use INIT_OBJ instead of memset + magic assignment

April sales extra exclusively to customers in Paris: Now with free
capitalized letters!

Only present if struct sockaddr.sa_len exists, to be checked with
preprocessor's #ifdef.

Refs #3154

As with any other out-of-workspace condition during ESI processing, we
do not have any better way than to deliver an incomplete response
(missing ESI include).

Or do we?

Fixes #3241

This should help locating the panicking thread in a core dump when when
the principle thread as determined by the debugger is a different one.

in varnish-cache, access to all ESI sub-requests happens in a single
thread, but vmods (VDPs) may add concurrency.

We thus protect access to PRIV_TOP with the session mutex.

Any vmods using this facility will likely need to add additional locking for
the actual data structures referenced through the PRIV_TOP and any other
access to the top request.

For alternatives previously considered, see #3139

VGZ_NewGzip will either assert or succeed.

Turn assertion into VFP error

The vtc is based upon r02645.vtc and reliably reproduces the panic
without the patch by sweeping through possible amounts of free workspace
ranging from 4 to 400 bytes.

Fixes #3253

After the initial discussion from #3163, and looking more closely at how
variable access is handled in subroutines I noticed a discrepancy.

Setting a read only variable like obj.ttl in vcl_recv would result in
a misleading error message explaining that it is read only, instead of
simply not available.

This change defers the illegal write check, registering unconditionally
that the symbol was used in a set action. As a result we always get the
correct error message but depending on whether this is happening in a
vcl_ or custom subroutine we may either get "in subroutine" or "from
subroutine" in the error message. A minor discrepancy probably worth
getting rid of the prior inconsistency.

This is covered by the v21 test case.

That would be the symbol itself instead of only the relevant mask, and a
XREF constants wrapping the error message as well. The `struct xrefuse`
pun was definitely intended.